[java] Fix avro logical-types conversions for BQ storage #33422

RustedBones · 2024-12-19T09:59:42Z

Most of the avro logical-type to BQ are broken.

Add support for both joda and java time to ensure compatibility with older avro versions

RustedBones · 2024-12-19T10:01:54Z

...rm/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/AvroGenericRecordToStorageApiProto.java

@@ -115,34 +120,90 @@ static String convertUUID(Object value) {
  }

  static Long convertTimestamp(Object value, boolean micros) {
-    if (value instanceof ReadableInstant) {
-      return ((ReadableInstant) value).getMillis() * (micros ? 1000 : 1);


This was wrong. BQ always expects epoch microseconds. Conversion should be applied on the raw type depending if it represents millis or micros

RustedBones · 2024-12-19T10:05:41Z

...rm/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/AvroGenericRecordToStorageApiProto.java

+              .setScale(type.getScale(), RoundingMode.DOWN)
+              .round(new MathContext(type.getPrecision(), RoundingMode.DOWN));


Does this seems legit to round ? We might also fail if the BigDecimal precision and scale do not match with expected logical type

I understand previously it only accepts a ByteBuffer, now it's adding support of java.math.BigDecimal? If previously it would fail or it's not lose precision compared to the existing behavior I think it's fine.

All other conversions accept the logical-type as well as the underlying-type.

This is mandatory to support both: even if the logical-type is present on the schema, it can be discarded when the GenericData used for serialization does not have the feature enabled.

Concerning the rounding, The doc states to use BigDecimalByteStringEncoder. BeamRowToStorageApiProto is a copy of that.

It's however not supporting parameterized NUMERIC and BIGNUMERIC types.

github-actions · 2024-12-19T11:05:54Z

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

RustedBones · 2024-12-19T11:15:30Z

assign set of reviewers

github-actions · 2024-12-19T11:16:44Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @kennknowles for label java.
R: @chamikaramj for label io.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

github-actions · 2024-12-26T12:14:11Z

Reminder, please take a look at this pr: @kennknowles @chamikaramj

github-actions · 2024-12-31T12:14:15Z

Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:

R: @Abacn for label java.
R: @johnjcasey for label io.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

github-actions · 2025-01-08T12:14:24Z

Reminder, please take a look at this pr: @Abacn @johnjcasey

Most of the avro logical-type to BQ are broken. Add support for both joda and java time to ensure compatibility with older avro versions

github-actions · 2025-01-13T12:14:55Z

Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:

R: @damondouglas for label java.
R: @damondouglas for label io.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

Abacn

Thanks for the change! Had a few questions.

Abacn · 2025-01-15T15:50:04Z

...rm/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/AvroGenericRecordToStorageApiProto.java

-          .put(LogicalTypes.timestampMicros().getName(), TableFieldSchema.Type.TIMESTAMP)
-          .put(LogicalTypes.timestampMillis().getName(), TableFieldSchema.Type.TIMESTAMP)
-          .put(LogicalTypes.uuid().getName(), TableFieldSchema.Type.STRING)
+          .put("date", TableFieldSchema.Type.DATE)


what's the reason to change them to hard coded names here? I understand they should be equivalent? Or keep the getName() while note the resolved name as comments?

They are equivalent. I changed because type name for decimal is not accessible and requires creation of a 'fake' logical-type.

LogicalTypes.decimal(1).getName()

I can revert to the old style if that's prefered.

Abacn · 2025-01-15T15:53:49Z

...rm/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/AvroGenericRecordToStorageApiProto.java

@@ -115,34 +120,97 @@ static String convertUUID(Object value) {
  }

  static Long convertTimestamp(Object value, boolean micros) {
-    if (value instanceof ReadableInstant) {
-      return ((ReadableInstant) value).getMillis() * (micros ? 1000 : 1);
+    if (value instanceof org.joda.time.ReadableInstant) {


may worth add a comment noting the following helper functions support both joda time and java.time, and refer to #19215 as future clean up

Abacn · 2025-01-15T15:57:26Z

...rm/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/AvroGenericRecordToStorageApiProto.java

+              .setScale(type.getScale(), RoundingMode.DOWN)
+              .round(new MathContext(type.getPrecision(), RoundingMode.DOWN));


I understand previously it only accepts a ByteBuffer, now it's adding support of java.math.BigDecimal? If previously it would fail or it's not lose precision compared to the existing behavior I think it's fine.

...rm/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/AvroGenericRecordToStorageApiProto.java

RustedBones · 2025-01-16T17:19:22Z

...rm/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/AvroGenericRecordToStorageApiProto.java

-                byteBuffer.duplicate(),
-                Schema.create(Schema.Type.NULL), // dummy schema, not used
-                logicalType);
-    return BeamRowToStorageApiProto.serializeBigDecimalToNumeric(bigDecimal);


This is considering the target column to be NUMERIC (which did not even matched with the inferred BIGNUMERIC type).

Avro decimal is parametrized, so are the NUMERIC and BIGNUMERIC. The proto bytes sent for this column must respect those parameters.

github-actions bot added java io gcp labels Dec 19, 2024

RustedBones commented Dec 19, 2024

View reviewed changes

RustedBones mentioned this pull request Dec 19, 2024

Add integration testing for BQ Storage API write modes #21477

Open

github-actions bot added the Next Action: Reviewers label Dec 19, 2024

github-actions bot added the slow-review label Dec 26, 2024

github-actions bot removed the slow-review label Dec 31, 2024

github-actions bot added the slow-review label Jan 8, 2025

RustedBones added 2 commits January 9, 2025 12:27

[java] Fix avro logical-types conversions for BQ storage

061a5f3

Most of the avro logical-type to BQ are broken. Add support for both joda and java time to ensure compatibility with older avro versions

expected int raw type for time-millis

f8d575b

RustedBones force-pushed the bq-storage-time branch from 30d0a2a to f8d575b Compare January 9, 2025 11:27

github-actions bot removed the slow-review label Jan 13, 2025

Abacn reviewed Jan 15, 2025

View reviewed changes

RustedBones added 3 commits January 16, 2025 15:20

Remove unused qualifier

e2e12c9

Fix avro numeric convertion

d59ec68

Add support for parametrized NUMERIC and BIGNUMERIC

2401a60

RustedBones commented Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[java] Fix avro logical-types conversions for BQ storage #33422

[java] Fix avro logical-types conversions for BQ storage #33422

RustedBones commented Dec 19, 2024

RustedBones Dec 19, 2024

RustedBones Dec 19, 2024 •

edited

Loading

Abacn Jan 15, 2025

RustedBones Jan 16, 2025

github-actions bot commented Dec 19, 2024

RustedBones commented Dec 19, 2024

github-actions bot commented Dec 19, 2024

github-actions bot commented Dec 26, 2024

github-actions bot commented Dec 31, 2024

github-actions bot commented Jan 8, 2025

github-actions bot commented Jan 13, 2025

Abacn left a comment

Abacn Jan 15, 2025

RustedBones Jan 16, 2025

Abacn Jan 15, 2025

Abacn Jan 15, 2025

RustedBones Jan 16, 2025

		.setScale(type.getScale(), RoundingMode.DOWN)
		.round(new MathContext(type.getPrecision(), RoundingMode.DOWN));

[java] Fix avro logical-types conversions for BQ storage #33422

Are you sure you want to change the base?

[java] Fix avro logical-types conversions for BQ storage #33422

Conversation

RustedBones commented Dec 19, 2024

Choose a reason for hiding this comment

RustedBones Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 19, 2024

RustedBones commented Dec 19, 2024

github-actions bot commented Dec 19, 2024

github-actions bot commented Dec 26, 2024

github-actions bot commented Dec 31, 2024

github-actions bot commented Jan 8, 2025

github-actions bot commented Jan 13, 2025

Abacn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RustedBones Dec 19, 2024 •

edited

Loading