gemini-code-assist[bot] commented on code in PR #37277:
URL: https://github.com/apache/beam/pull/37277#discussion_r2678624353
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rel/BeamCalcRel.java:
##########
@@ -692,6 +769,47 @@ private static Expression toCalciteValue(Expression value,
FieldType fieldType)
return nullOr(value, returnValue);
} else if (FixedPrecisionNumeric.IDENTIFIER.equals(identifier)) {
return Expressions.convert_(value, BigDecimal.class);
+ } else if (fieldType.getLogicalType() instanceof
PassThroughLogicalType) {
+ // For pass-through logical types, convert underlying base type to
Calcite value
+ FieldType baseType = fieldType.getLogicalType().getBaseType();
+ switch (baseType.getTypeName()) {
+ case BYTE:
+ return Expressions.convert_(value, Byte.class);
+ case INT16:
+ return Expressions.convert_(value, Short.class);
+ case INT32:
+ return Expressions.convert_(value, Integer.class);
+ case INT64:
+ return Expressions.convert_(value, Long.class);
+ case DECIMAL:
+ return Expressions.convert_(value, BigDecimal.class);
+ case FLOAT:
+ return Expressions.convert_(value, Float.class);
+ case DOUBLE:
+ return Expressions.convert_(value, Double.class);
+ case STRING:
+ return Expressions.convert_(value, String.class);
+ case BOOLEAN:
+ return Expressions.convert_(value, Boolean.class);
+ case DATETIME:
+ return nullOr(
+ value,
+ Expressions.call(
+ Expressions.convert_(value, AbstractInstant.class),
"getMillis"));
+ case BYTES:
+ return nullOr(
+ value,
+ Expressions.new_(ByteString.class,
Expressions.convert_(value, byte[].class)));
+ case ARRAY:
+ return nullOr(value, toCalciteList(value,
baseType.getCollectionElementType()));
+ case MAP:
+ return nullOr(value, toCalciteMap(value,
baseType.getMapValueType()));
+ case ROW:
+ return nullOr(value, toCalciteRow(value,
baseType.getRowSchema()));
+ default:
+ throw new UnsupportedOperationException(
+ "Unable to convert logical type " + identifier);
+ }
Review Comment:
Similar to the change in `getBeamField`, this large block for handling
`PassThroughLogicalType` is an undocumented refactoring that is out of scope
for this pull request.
While this block is placed after the specific handlers for
`PassThroughLogicalType`s like `FixedPrecisionNumeric` (unlike in
`getBeamField`), its presence is still a significant, unrelated change.
For clarity and to keep this PR focused, please remove this refactoring and
propose it in a separate pull request.
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rel/BeamCalcRel.java:
##########
@@ -601,6 +618,59 @@ private static Expression getBeamField(
fieldName,
Expressions.constant(LocalDateTime.class)),
LocalDateTime.class);
+ } else if (fieldType.getLogicalType() instanceof
PassThroughLogicalType) {
+ // For pass-through logical types, read the underlying base type
using the
+ // corresponding Row getter.
+ FieldType baseType = fieldType.getLogicalType().getBaseType();
+ switch (baseType.getTypeName()) {
+ case BYTE:
+ value = Expressions.call(expression, "getByte", fieldName);
+ break;
+ case INT16:
+ value = Expressions.call(expression, "getInt16", fieldName);
+ break;
+ case INT32:
+ value = Expressions.call(expression, "getInt32", fieldName);
+ break;
+ case INT64:
+ value = Expressions.call(expression, "getInt64", fieldName);
+ break;
+ case DECIMAL:
+ value = Expressions.call(expression, "getDecimal", fieldName);
+ break;
+ case FLOAT:
+ value = Expressions.call(expression, "getFloat", fieldName);
+ break;
+ case DOUBLE:
+ value = Expressions.call(expression, "getDouble", fieldName);
+ break;
+ case STRING:
+ value = Expressions.call(expression, "getString", fieldName);
+ break;
+ case DATETIME:
+ value = Expressions.call(expression, "getDateTime", fieldName);
+ break;
+ case BOOLEAN:
+ value = Expressions.call(expression, "getBoolean", fieldName);
+ break;
+ case BYTES:
+ value = Expressions.call(expression, "getBytes", fieldName);
+ break;
+ case ARRAY:
+ value = Expressions.call(expression, "getArray", fieldName);
+ break;
+ case MAP:
+ value = Expressions.call(expression, "getMap", fieldName);
+ break;
+ case ROW:
+ value = Expressions.call(expression, "getRow", fieldName);
+ break;
+ case ITERABLE:
+ value = Expressions.call(expression, "getIterable", fieldName);
+ break;
+ default:
+ throw new UnsupportedOperationException("Unable to get logical
type " + identifier);
+ }
Review Comment:
This large block of code for handling `PassThroughLogicalType` seems to be
an unrelated refactoring. It's not mentioned in the pull request description
and significantly increases the scope of the change.
Furthermore, this change introduces an issue. `FixedPrecisionNumeric` is a
`PassThroughLogicalType`, so it will be handled by this new generic block. This
makes the specific `else if` block for `FixedPrecisionNumeric` on line 674
unreachable.
This refactoring is also inconsistent with the changes in `toCalciteValue`,
where the new `PassThroughLogicalType` block is placed *after* the specific
`FixedPrecisionNumeric` block.
It would be best to remove this refactoring from this PR and submit it as a
separate change with a proper description and testing. This PR should focus
only on adding support for `micros_instant` and `nanos_instant`.
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rel/BeamCalcRel.java:
##########
@@ -416,6 +418,10 @@ static Object toBeamObject(Object value, FieldType
fieldType, boolean verifyValu
String identifier = logicalType.getIdentifier();
if (TimeWithLocalTzType.IDENTIFIER.equals(identifier)) {
return Instant.ofEpochMilli(((Number) value).longValue());
+ } else if (MicrosInstant.IDENTIFIER.equals(identifier)
+ || NanosInstant.IDENTIFIER.equals(identifier)) {
+ // Portable instant logical types: treat calcite numeric as epoch
milliseconds.
+ return Instant.ofEpochMilli(((Number) value).longValue());
Review Comment:
The logical types `MicrosInstant` and `NanosInstant` are defined for
`java.time.Instant`. However, this code returns an `org.joda.time.Instant`
because of the existing import for `org.joda.time.Instant`. This will cause a
type mismatch when creating the `Row`.
Please change this to return a `java.time.Instant`. You may need to use a
fully qualified name to avoid import conflicts.
```suggestion
// Portable instant logical types: treat calcite numeric as epoch
milliseconds.
return java.time.Instant.ofEpochMilli(((Number)
value).longValue());
```
--
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]
