-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Update BigQuery storage read API usage to v1 #10035
Conversation
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.
Almost good to me. Do you know whether a retryable message of BigQueryUtil.INTERNAL_ERROR_MESSAGES
is valid after updating to v1?
plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryPageSourceProvider.java
Outdated
Show resolved
Hide resolved
plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQuerySplitManager.java
Outdated
Show resolved
Hide resolved
plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/ReadSessionCreator.java
Outdated
Show resolved
Hide resolved
The definitions are still valid, although they may no longer be required since some of this logic is now implemented directly in the client (see e.g. here). Would you like me to remove them, either as part of this PR or later? |
It should also be safe to remove e.g. BigQueryUtils.validColumnName, since the read API now supports BigQuery pseudo-columns. |
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.
skimmed. couple of comments. I'm reviewing changes between the API versions to see if there are any other functional changes we should make on our end.
plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/ReadRowsHelper.java
Outdated
Show resolved
Hide resolved
plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/ReadSessionCreator.java
Show resolved
Hide resolved
plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestReadRowsHelper.java
Show resolved
Hide resolved
plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/ReadSessionCreator.java
Show resolved
Hide resolved
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.
Please squash commits into one.
Update BigQuery storage protos to v1 Update plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQuerySplitManager.java Co-authored-by: Yuya Ebihara <ebyhry@gmail.com> Update plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryPageSourceProvider.java Co-authored-by: Yuya Ebihara <ebyhry@gmail.com> Update plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/ReadSessionCreator.java Co-authored-by: Yuya Ebihara <ebyhry@gmail.com> Fix code review feedback: rename member variable Revert unneeded pom.xml changes
bb615bc
to
33f2fff
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.
LGTM % comment
plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestReadRowsHelper.java
Show resolved
Hide resolved
Cherry-pick of trinodb/trino#10035 Co-authored-by: Kenneth Jung <kmjung@gmail.com>
This change updates the BigQuery connector to use the v1 interface for the BigQuery read API in preparation for the eventual turndown of v1beta1.