github-actions[bot] commented on code in PR #40770:
URL: https://github.com/apache/beam/pull/40770#discussion_r4368665832
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowJson.java:
##########
@@ -602,15 +582,17 @@ private void writeValue(JsonGenerator gen, FieldType
type, Object value) throws
gen.writeEndObject();
break;
case LOGICAL_TYPE:
- String identifier = type.getLogicalType().getIdentifier();
+ Schema.LogicalType<Object, Object> logicalType =
+ (Schema.LogicalType<Object, Object>) type.getLogicalType();
+ String identifier = logicalType.getIdentifier();
if (SqlTypes.DATE.getIdentifier().equals(identifier)) {
gen.writeString(((LocalDate) value).toString()); // ISO 8601 format
} else if (SqlTypes.TIME.getIdentifier().equals(identifier)) {
gen.writeString(((LocalTime) value).toString()); // ISO 8601 format
} else if (SqlTypes.DATETIME.getIdentifier().equals(identifier)) {
gen.writeString(((LocalDateTime) value).toString()); // ISO 8601
format
} else {
- writeValue(gen, type.getLogicalType().getBaseType(), value);
+ writeValue(gen, logicalType.getBaseType(),
logicalType.toBaseType(value));
Review Comment:
**🟢 Low**: While `RowJsonDeserializer` recursively unwraps chained logical
types via `extractJsonValue(fieldName, logicalType.getBaseType(), jsonParser)`
(unless the inner type is one of `SqlTypes.DATE`/`TIME`/`DATETIME`, which
expects the input type rather than the base type), `RowJsonSerializer` calls
`writeValue(gen, logicalType.getBaseType(), logicalType.toBaseType(value))`. If
a custom `LogicalType` wraps `SqlTypes.DATE`, `SqlTypes.TIME`, or
`SqlTypes.DATETIME` as its base type, `deserializer` will extract a
`LocalDate`/`LocalTime`/`LocalDateTime` and pass it to the outer
`logicalType.toInputType(...)`, which works as expected—just something to keep
in mind.
##########
sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/JsonToRowTest.java:
##########
@@ -317,6 +317,9 @@ public void
testParsesErrorWithErrorMsgRowsDeadLetterWithCustomFieldNames() thro
PCollection<Row> personRows = results.getResults();
PCollection<Row> errorsWithMsg = results.getFailedToParseLines();
+ org.hamcrest.MatcherAssert.assertThat(
Review Comment:
**🟢 Nit**: Consider static-importing `MatcherAssert.assertThat` and
`Matchers.equalTo` for consistency with other test classes in this package
(e.g., `RowJsonTest`).
--
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]
