Abacn commented on code in PR #34347:
URL: https://github.com/apache/beam/pull/34347#discussion_r2063838648
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowJsonUtils.java:
##########
@@ -103,4 +107,46 @@ public static String rowToJson(ObjectMapper objectMapper,
Row row) {
throw new IllegalArgumentException("Unable to serialize row: " + row, e);
}
}
+
+ /**
+ * Verifies that all field types in the schema are supported by RowJson
deserialization. Throws an
+ * IllegalArgumentException if an unsupported type is encountered.
+ */
+ public static void verifySchemaSupported(Schema schema) {
Review Comment:
is this method unused?
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowJson.java:
##########
@@ -148,12 +152,22 @@ private static ImmutableList<UnsupportedField>
findUnsupportedFields(
TypeName fieldTypeName = fieldType.getTypeName();
if (fieldTypeName.isCompositeType()) {
- return fieldType.getRowSchema().getFields().stream()
- .flatMap(
- (field) ->
- findUnsupportedFields(field.getType(), fieldName + "." +
field.getName())
- .stream())
- .collect(toImmutableList());
+ if (fieldTypeName == TypeName.ROW) {
+ return fieldType.getRowSchema().getFields().stream()
+ .flatMap(
+ (field) ->
+ findUnsupportedFields(field.getType(), fieldName + "." +
field.getName())
+ .stream())
+ .collect(toImmutableList());
+ } else if (fieldTypeName == TypeName.MAP) {
Review Comment:
I am not sure if this could change current behavior. To be safe and avoid
breaking change how about
```
if (fieldTypeName == TypeName.MAP) {
....
return ...
}
<current code>
```
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/JsonToRow.java:
##########
@@ -57,6 +57,10 @@
* <li>{@link Schema.TypeName#DOUBLE}
* <li>{@link Schema.TypeName#BOOLEAN}
* <li>{@link Schema.TypeName#STRING}
+ * <li>{@link Schema.TypeName#DECIMAL}
+ * <li>{@link Schema.TypeName#DATETIME}
+ * <li>{@link Schema.TypeName#MAP} (keys must be STRING; values must be
BYTE, INT16, INT32, INT64,
Review Comment:
I understand the code does (and json spec as well) does not explicitly
restricts value type of a map
(https://github.com/apache/beam/pull/34347/files#diff-f6962eeb7e5675e74f35dd7c94c4550bbd06f62cef7d3067e85324238db75525R597).
Consider just phrases as "value must be a supported field type"
--
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: github-unsubscr...@beam.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
