-
Notifications
You must be signed in to change notification settings - Fork 4.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
Improve BQ <-> Avro conversions #32482
Conversation
c2055eb
to
55017e6
Compare
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
55017e6
to
2312dbe
Compare
Assigning reviewers. If you would like to opt out of this review, comment R: @robertwb for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
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.
Thanks for this effort, went through once and had comments. For BQIO changes, @ahmedabu98 could you please also take a look? Thanks!
...-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryAvroUtilsTest.java
Show resolved
Hide resolved
...ogle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryAvroUtils.java
Show resolved
Hide resolved
...ogle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryAvroUtils.java
Show resolved
Hide resolved
...ogle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryAvroUtils.java
Show resolved
Hide resolved
...ud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryStorageSourceBase.java
Outdated
Show resolved
Hide resolved
...o/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java
Outdated
Show resolved
Hide resolved
...a/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIO.java
Outdated
Show resolved
Hide resolved
...ogle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQuerySourceDef.java
Outdated
Show resolved
Hide resolved
...ogle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryAvroUtils.java
Show resolved
Hide resolved
...ud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOStorageReadTest.java
Outdated
Show resolved
Hide resolved
e22368a
to
a4ad8d3
Compare
a4ad8d3
to
e1f6d5a
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.
+1 thanks for this effort. Left a few comments but the rest looks great
...ogle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryAvroUtils.java
Outdated
Show resolved
Hide resolved
...ogle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryAvroUtils.java
Outdated
Show resolved
Hide resolved
...ogle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryAvroUtils.java
Show resolved
Hide resolved
...ogle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryAvroUtils.java
Outdated
Show resolved
Hide resolved
...ogle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryAvroUtils.java
Outdated
Show resolved
Hide resolved
// in Extract Jobs, it always uses the Avro logical type | ||
// we may have to change this if we move to EXPORT DATA | ||
return LogicalTypes.timestampMicros().addToSchema(SchemaBuilder.builder().longType()); |
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.
useAvroLogicalTypes
has a different behavior when used in extract job vs export data for TIMESTAMP
columns.
Currently, beam's implementation of query is relying on temp table + extract job, so we don't have to handle this discrepancy.
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
Failing test is unrelated, merging now |
Extracted from #32360
This change-set focuses on conversions between BQ
TableSchema
/TableRow
and AvroSchema
/GenericRecord
Fix #20677