-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(bigquery): add create_bqstorage_client
param to to_dataframe
and to_arrow
#9573
Conversation
feedback during informal review: be clearer we're only using default settings when constructing the client on user behalf, and that to do more you'll still want to supply your own client. |
use_bqstorage_api
param to to_dataframe
and `…create_bqstorage_client
param to to_dataframe
and to_arrow
dcbf7dd
to
119f19b
Compare
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.
Looks good, I only have a few minor comments.
Making it more clear that auto-creating a BQ storage client behind the scenes only uses the default settings would indeed be beneficial, as @shollyman pointed out.
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.
The remaining remark is trivial, feel free to ignore, as the linter appears to not bother.
|
||
download_public_data_sandbox.download_public_data_sandbox(client) | ||
out, _ = capsys.readouterr() | ||
out, err = capsys.readouterr() |
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.
err
is not used anywhere?
Per #9457 I should close the client transport if we create it in the method. |
create_bqstorage_client
param to to_dataframe
and to_arrow
create_bqstorage_client
param to to_dataframe
and to_arrow
… and `to_arrow` When the `create_bqstorage_client` parameter is set to `True`, the BigQuery client constructs a BigQuery Storage API client for you. This removes the need for boilerplate code to manually construct both clients explitly with the same credentials. Does this make the `bqstorage_client` parameter unnecessary? In most cases, yes, but there are a few cases where we'll want to continue using it. * When partner tools use `to_dataframe`, they should continue to use `bqstorage_client` so that they can set the correct amended user-agent strings. * When a developer needs to override the default API endpoint for the BQ Storage API, they'll need to manually supply a `bqstorage_client`.
75baaad
to
bf00d45
Compare
create_bqstorage_client
param to to_dataframe
and to_arrow
create_bqstorage_client
param to to_dataframe
and to_arrow
When the
create_bqstorage_client
parameter is set toTrue
, the BigQueryclient constructs a BigQuery Storage API client for you. This removes
the need for boilerplate code to manually construct both clients
explicitly with the same credentials.
Does this make the
bqstorage_client
parameter unnecessary? In mostcases, yes, but there are a few cases where we'll want to continue using
it. Specifically, when partner tools use
to_dataframe
, they shouldcontinue to use
bqstorage_client
so that they can set the correctamended user-agent strings.
bqstorage_client
is also needed for regionalAPI endpoints.
TODO
owns_bqstorage_client
to track ifbqstorage_client
needs its transport closed.owns_bqstorage_client
is true.max_results
is set.