- Notifications
You must be signed in to change notification settings - Fork1.6k
BigQuery: Add ability to pass in a table ID instead of a query to the %%bigquery magic.#9170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
BigQuery: Add ability to pass in a table ID instead of a query to the %%bigquery magic.#9170
Uh oh!
There was an error while loading.Please reload this page.
Conversation
added default patch to unit tests
6761217 to77e62c5Compare| rows=client.list_rows(table_id,max_results=max_results) | ||
| exceptExceptionasex: | ||
| error=str(ex) | ||
| iferror: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
could this be simplified to return from the exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it definitely can. fixed in7a13ca4
| error=None | ||
| ifnotre.search(r"\s",query.rstrip()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What scenario is this protecting against? Is there a scenario where unicode strings will not remove space characters via rstrip?
What do we expect in the else case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Related: Does this repository enforce code coverage? Is there a case where we test there being unstoppable whitespace and not taking this branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This condition is actually testing for queries that contain whitespace characters that aren't removed by rstrip (so any whitespace that isn't a trailing newline). The assumption being made (as described in#9105 ) is that anything containing whitespace is a SQL query and won't take this branch, while anything string without whitespace is a table_id and will take this branch.
So anything regular SQL query would fall into the else case. There are tests that check whether query strings without spaces will be interpreted as table IDs and a test that checks if a string without whitespace that isn't a valid table_id raises an appropriate error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I can add a comment in the code if that would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would also write this down in a comment, i.e. that anything without whitespace is assumed to be table identifier which triggers a different use case of the magic command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
added comment ind663104
plamut left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The change generally works.
I did notice, however, that the logic is sensitive to theleading whitespace:
┌───────────────────────────────────────────┐│ %%bigquery --max_results=6 │ | bigquery-public-data.samples.shakespeare │└───────────────────────────────────────────┘ Executing query with job ID: a0f195e6-f748-4c42-8c3b-d89abaa37c9f Query executing: 0.92s ERROR: 400 Syntax error: Unexpected identifier "bigquery" at [1:3]Wouldn't it be better to strip the whitespace on both sides first, or why we only do.rstrip()?
| query_job=_run_query(client,query,job_config=job_config) | ||
| exceptExceptionasex: | ||
| error=str(ex) | ||
| return_print_error(str(ex),args.destination_var) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This can be slightly confusing for the reader at first glance, because_print_error() itself does not return anything, it just has a side effect. I would simply express the same in two lines (a solereturn in its own).
shubha-rajanSep 21, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed in6f2dd64
| error=None | ||
| ifnotre.search(r"\s",query.rstrip()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would also write this down in a comment, i.e. that anything without whitespace is assumed to be table identifier which triggers a different use case of the magic command.
| max_results=None | ||
| ifnotre.search(r"\s",query.rstrip()): | ||
| table_id=query.rstrip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
BTW, stripping whitespace from the same value again is not necessary, we could store the stripped string into a variable the first time we do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed ind663104
plamut left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, thanks!
It might cause a merge conflict with#9245, but I'll address that there.
Uh oh!
There was an error while loading.Please reload this page.
Third of 3 PRs towards resolving#9105 as described in review for#9147
Screenshot of feature in notebook: