gemini-code-assist[bot] commented on code in PR #37235:
URL: https://github.com/apache/beam/pull/37235#discussion_r2666127964
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/AvroGenericRecordToStorageApiProto.java:
##########
@@ -502,10 +520,42 @@ static Object mapEntryToProtoValue(
return builder.build();
}
+ private static DynamicMessage buildTimestampPicosMessage(
+ Descriptor timestampPicosDescriptor, long seconds, long picoseconds) {
+ return DynamicMessage.newBuilder(timestampPicosDescriptor)
+ .setField(
+
Preconditions.checkNotNull(timestampPicosDescriptor.findFieldByName("seconds")),
+ seconds)
+ .setField(
+
Preconditions.checkNotNull(timestampPicosDescriptor.findFieldByName("picoseconds")),
+ picoseconds)
+ .build();
+ }
+
@VisibleForTesting
- static Object scalarToProtoValue(Schema fieldSchema, Object value) {
+ static Object scalarToProtoValue(
+ @Nullable FieldDescriptor descriptor, Schema fieldSchema, Object value) {
TypeWithNullability type = TypeWithNullability.create(fieldSchema);
+ if
(TIMESTAMP_NANOS_LOGICAL_TYPE.equals(type.getType().getProp("logicalType"))) {
+ Preconditions.checkArgument(
+ value instanceof Long, "Expecting a value as Long type
(timestamp-nanos).");
+ long nanos = (Long) value;
+
+ long seconds = nanos / NANOS_PER_SECOND;
+ long nanoAdjustment = nanos % NANOS_PER_SECOND;
+
+ // Handle negative timestamps (before epoch)
+ if (nanos < 0 && nanoAdjustment != 0) {
+ seconds -= 1;
+ nanoAdjustment += NANOS_PER_SECOND;
+ }
Review Comment:
The logic to handle negative timestamps can be simplified by using
`Math.floorDiv` and `Math.floorMod`. This makes the code more concise and less
error-prone, as it directly implements the desired behavior for division and
remainder with negative numbers.
```suggestion
long seconds = Math.floorDiv(nanos, NANOS_PER_SECOND);
long nanoAdjustment = Math.floorMod(nanos, NANOS_PER_SECOND);
```
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BeamRowToStorageApiProto.java:
##########
@@ -341,17 +358,39 @@ private static Object toProtoValue(
fieldDescriptor.getMessageType(), keyType, valueType,
entry))
.collect(Collectors.toList());
default:
- return scalarToProtoValue(beamFieldType, value);
+ return scalarToProtoValue(fieldDescriptor, beamFieldType, value);
}
}
+ private static DynamicMessage buildTimestampPicosMessage(
+ Descriptor timestampPicosDescriptor, Instant instant) {
+ long seconds = instant.getEpochSecond();
+ long picoseconds = instant.getNano() * 1000L; // nanos → picos
Review Comment:
To improve readability and maintainability, the magic number `1000L` should
be replaced with a named constant like `PICOS_PER_NANO`. This constant could be
defined at the class level.
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BeamRowToStorageApiProto.java:
##########
@@ -243,9 +245,24 @@ private static TableFieldSchema
fieldDescriptorFromBeamField(Field field) {
if (logicalType == null) {
throw new RuntimeException("Unexpected null logical type " +
field.getType());
}
- @Nullable TableFieldSchema.Type type =
LOGICAL_TYPES.get(logicalType.getIdentifier());
- if (type == null) {
- throw new RuntimeException("Unsupported logical type " +
field.getType());
+ @Nullable TableFieldSchema.Type type;
+ if (logicalType.getIdentifier().equals(Timestamp.IDENTIFIER)) {
+ int precision =
+ Preconditions.checkNotNull(
+ logicalType.getArgument(),
+ "Expected logical type argument for timestamp precision.");
+ if (precision != 9) {
+ throw new RuntimeException(
+ "Unsupported precision for Timestamp logical type " +
precision);
+ }
+ // Map Timestamp.NANOS logical type to BigQuery TIMESTAMP(12) for
nanosecond precision
+ type = TableFieldSchema.Type.TIMESTAMP;
+
builder.setTimestampPrecision(Int64Value.newBuilder().setValue(12L).build());
Review Comment:
To improve readability and maintainability, the magic number `12L` should be
replaced with a named constant like `PICOSECOND_PRECISION`. This constant could
be defined at the class level.
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java:
##########
@@ -587,7 +587,17 @@ private static List<TableFieldSchema>
toTableFieldSchema(Schema schema) {
field.setFields(toTableFieldSchema(mapSchema));
field.setMode(Mode.REPEATED.toString());
}
- field.setType(toStandardSQLTypeName(type).toString());
+ Schema.LogicalType<?, ?> logicalType = type.getLogicalType();
+ if (logicalType != null &&
Timestamp.IDENTIFIER.equals(logicalType.getIdentifier())) {
+ int precision =
Preconditions.checkArgumentNotNull(logicalType.getArgument());
+ if (precision != 9) {
+ throw new IllegalArgumentException(
+ "Unsupported precision for Timestamp logical type " + precision);
+ }
+
field.setType(StandardSQLTypeName.TIMESTAMP.toString()).setTimestampPrecision(12L);
Review Comment:
To improve readability and maintainability, the magic number `12L` should be
replaced with a named constant like `PICOSECOND_PRECISION`. This constant could
be defined at the class level.
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryAvroUtils.java:
##########
@@ -644,7 +644,7 @@ private static TableFieldSchema
typedTableFieldSchema(Schema type, Boolean useAv
// TODO: Use LogicalTypes.TimestampNanos once avro version is updated.
if (useAvroLogicalTypes
&&
(TIMESTAMP_NANOS_LOGICAL_TYPE.equals(type.getProp("logicalType")))) {
- return fieldSchema.setType("TIMESTAMP");
+ return fieldSchema.setType("TIMESTAMP").setTimestampPrecision(12L);
Review Comment:
To improve readability and maintainability, the magic number `12L` should be
replaced with a named constant like `PICOSECOND_PRECISION`. This constant could
be defined at the class level.
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryTimestampPicosIT.java:
##########
@@ -392,6 +398,112 @@ public void testReadWithNanosPrecision_Arrow() {
runReadTest(TimestampPrecision.NANOS, DataFormat.ARROW, expectedOutput,
simpleTableSpec);
}
+ // Schema with custom timestamp-nanos logical type
+ private static org.apache.avro.Schema createTimestampNanosAvroSchema() {
+ org.apache.avro.Schema longSchema =
+ org.apache.avro.Schema.create(org.apache.avro.Schema.Type.LONG);
+ longSchema.addProp("logicalType", "timestamp-nanos");
+ return org.apache.avro.SchemaBuilder.record("TimestampNanosRecord")
+ .fields()
+ .name("ts_nanos")
+ .type(longSchema)
+ .noDefault()
+ .endRecord();
+ }
+
+ private static final java.time.Instant TEST_INSTANT =
+ java.time.Instant.parse("2024-01-15T10:30:45.123456789Z");
+
+ private static final org.apache.avro.Schema TIMESTAMP_NANOS_AVRO_SCHEMA =
+ createTimestampNanosAvroSchema();
+
+ @Test
+ public void testWriteGenericRecordTimestampNanos() throws Exception {
+ String tableSpec =
+ String.format("%s:%s.%s", project, DATASET_ID,
"generic_record_ts_nanos_test");
+
+ // Create GenericRecord with timestamp-nanos value
+ GenericRecord record =
+ new GenericRecordBuilder(TIMESTAMP_NANOS_AVRO_SCHEMA)
+ .set(
+ "ts_nanos", TEST_INSTANT.getEpochSecond() * 1_000_000_000L +
TEST_INSTANT.getNano())
Review Comment:
To improve readability and maintainability, the magic number
`1_000_000_000L` should be replaced with a named constant like
`NANOS_PER_SECOND`. This constant could be defined at the class level.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
