- Notifications
You must be signed in to change notification settings - Fork1.6k
BigQuery: Add list rows and --max_results option to %%bigquery magic#9147
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 list rows and --max_results option to %%bigquery magic#9147
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
| :type api_response: dict | ||
| :paramapi_response: response returned from an API call | ||
| Args: | ||
| api_response (dict): response returned from an API call. |
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.
By the way, the types are specified with names from thetyping module, thus adict should be namedDict, for example. OrDict[key_type, value_type] if you also want to specify the dict content's type(s). Probably best to check some of the existing "modern" docstrings in the codebase to get a feel.
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.
got it. Thedict in question would be a nested API response so it would be okay to just name itDict without specifying the content, right?
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 suppose so, yes. If all keys are strings,Dict[str, Any] could be used, but justDict with a meaningful description is fine, too.
shubha-rajan commentedAug 30, 2019
failing snippets tests also fail locally on master, so they're probably unrelated to changes in this PR |
plamut commentedAug 31, 2019
@shubha-rajan Indeed, that started occurring a day or two ago. The backend team has been informed about it, we are awaiting the ETA for the fix. If it's too long, we can temporarily disable the failing test as a workaround. |
tswast commentedSep 3, 2019
Since these are two different features, let's have them as two (possibly 3) separate PRs, starting with I'd prefer we find a different implementation for Let's have 3 PRs in this order:
|
shubha-rajan commentedSep 3, 2019
@tswast sounds good. I'll close this one and submit 3 separate PRs |
Uh oh!
There was an error while loading.Please reload this page.
See#9105. Creating draft for visibility.
Adds option to pass a table id instead of a SQL query to
%%bigquerycell magic as a cost-saving alternative toSELECT *queries.--max_resultsoption limits the number of rows read. The returnedpandas.DataFramecan be saved to a variable by passing adestination_varargument.TODO:
Fix coverage failures
Test that running cell magic with table ID instead of query works with
bqstorage_clientsetAdd tests for failure cases- handles failure cases when table IDs are passed instead of queries:
max_resultsis currently not working with regular SQL queriesTo get this working, I ended up adding
max_resultsas a property ofQueryJobConfig, but if that wasn't the right call, I can refactor to passmax_resultsas a separate parameter.