-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Improve Integer conversion in BigQueryIO's Storage Write API method. #24366
Conversation
Assigning reviewers. If you would like to opt out of this review, comment R: @apilloud for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
try { | ||
return Long.valueOf((String) value); | ||
} catch (NumberFormatException e) { | ||
// Expected. Element will be added to the failed element PCollection |
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.
These are going to result in a generic SchemaDoesntMatchException("Unexpected value :"...)
being thrown which will make this potentially difficult to debug when it happens. If we can't just let the exception bubble up here, you should wrap it with a new exception rather than just dropping it. (Same thing for the exceptions below.)
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.
Addressed in the latest commit. Only implemented for the INTEGER type at the moment.
…Introduced a dedicated Exception. Improved error messages by capturing the source failure and reducing message sizes.
…Introduced a dedicated Exception; improved error messages by capturing the source failure.
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
# Conflicts: # sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/TableRowToStorageApiProto.java # sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/TableRowToStorageApiProtoTest.java
Reminder, please take a look at this pr: @apilloud @ahmedabu98 |
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @lukecwik for label java. Available commands:
|
...ud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/TableRowToStorageApiProto.java
Outdated
Show resolved
Hide resolved
…am/sdk/io/gcp/bigquery/TableRowToStorageApiProto.java
remind me after tests pass |
Ok - I'll remind @lukecwik after tests pass |
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.
Apply spotless
...ud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/TableRowToStorageApiProto.java
Outdated
Show resolved
Hide resolved
...latform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/TableRowToStorageApiProtoTest.java
Outdated
Show resolved
Hide resolved
...latform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/TableRowToStorageApiProtoTest.java
Outdated
Show resolved
Hide resolved
Apply spotless
retest this please |
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.
Fix weird merge
...ud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/TableRowToStorageApiProto.java
Outdated
Show resolved
Hide resolved
…am/sdk/io/gcp/bigquery/TableRowToStorageApiProto.java
…pache#24366) * Improve Integer conversion in BigQueryIO's Storage Write API method. * Improve Integer conversion in BigQueryIO's Storage Write API method. Introduced a dedicated Exception. Improved error messages by capturing the source failure and reducing message sizes. * Improve Integer conversion in BigQueryIO's Storage Write API method. Introduced a dedicated Exception; improved error messages by capturing the source failure. * Fixed a minor typo. * Merged PR with new commits. * Update sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/TableRowToStorageApiProto.java * Apply suggestions from code review Apply spotless * Update sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/TableRowToStorageApiProto.java Co-authored-by: slilichenko <Sergei Lilichenko> Co-authored-by: Lukasz Cwik <lcwik@google.com>
Improve error handling when converting TableRows to a schema containing BigQuery's INTEGER type: