gemini-code-assist[bot] commented on code in PR #38510:
URL: https://github.com/apache/beam/pull/38510#discussion_r3248847275
##########
sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/TableSchema.java:
##########
@@ -76,6 +78,22 @@ public static Schema.FieldType
getEquivalentFieldType(ColumnType columnType) {
case DATETIME:
return Schema.FieldType.DATETIME;
+ case DATETIME64:
+ // Pick the narrowest Beam logical type that still round-trips the
requested precision:
+ // ≤ 3 (milliseconds) → Joda DATETIME, keeping existing
pipelines unchanged.
+ // 4–6 (down to microseconds) → SqlTypes.TIMESTAMP (MicrosInstant) —
interoperable
+ // with BigQueryIO, Avro and Beam SQL.
+ // ≥ 7 (sub-microsecond) → NanosInstant, the only built-in type
that preserves
+ // full nanosecond precision through
Row construction.
+ int p = columnType.precision();
+ if (p <= 3) {
+ return Schema.FieldType.DATETIME;
+ } else if (p <= 6) {
+ return Schema.FieldType.logicalType(SqlTypes.TIMESTAMP);
+ } else {
+ return Schema.FieldType.logicalType(new NanosInstant());
Review Comment:
For better performance and consistency with other logical types (like
`SqlTypes.TIMESTAMP`), consider defining a private constant for `NanosInstant`
instead of instantiating it every time `getEquivalentFieldType` is called for a
`DATETIME64` column with precision ≥ 7.
```suggestion
return Schema.FieldType.logicalType(NANOS_INSTANT);
```
##########
sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/ClickHouseWriter.java:
##########
@@ -138,6 +171,10 @@ static void writeValue(ClickHouseOutputStream stream,
ColumnType columnType, Obj
BinaryStreamUtils.writeUnsignedInt32(stream, epochSeconds);
break;
+ case DATETIME64:
+ BinaryStreamUtils.writeInt64(stream, encodeDateTime64(value,
columnType.precision()));
Review Comment:
The `precision()` method on `ColumnType` is marked as `@Nullable`. While the
factory and parser ensure it is set for `DATETIME64`, unboxing it here to an
`int` for the `encodeDateTime64` call could theoretically throw a
`NullPointerException` if a `ColumnType` was manually constructed via the
builder without a precision. Consider adding a null check or using
`Objects.requireNonNull` for robustness.
```suggestion
BinaryStreamUtils.writeInt64(stream, encodeDateTime64(value,
java.util.Objects.requireNonNull(columnType.precision())));
```
--
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]
