wombatu-kun commented on code in PR #16673:
URL: https://github.com/apache/iceberg/pull/16673#discussion_r3353172647
##########
core/src/main/java/org/apache/iceberg/SchemaParser.java:
##########
@@ -264,14 +272,18 @@ private static Types.ListType listFromJson(JsonNode json)
{
}
}
- private static Types.MapType mapFromJson(JsonNode json) {
+ private static Types.MapType mapFromJson(JsonNode json, String fieldName) {
int keyId = JsonUtil.getInt(KEY_ID, json);
- Type keyType = typeFromJson(JsonUtil.get(KEY, json));
+ String keyName = childName(fieldName, KEY);
+ Type keyType = typeFromJson(JsonUtil.get(KEY, json), keyName);
+ validateUnknownFieldOptional(keyName, true, keyType);
Review Comment:
`validateUnknownFieldOptional` always advises `must be optional`, but a map
key can never be made optional: `MapType` builds the key as
`NestedField.required(keyId, "key", ...)` in both `ofOptional` and
`ofRequired`. So for the key case this message points the user at a fix the
type system forbids, which undercuts the goal of clearer diagnostics. Consider
a key-specific message such as `Unknown type cannot be used for map key '%s'`
(for example a small dedicated check for keys instead of reusing the
optional-oriented one). The assertion in `unknownMapKeyFailsWithKeyName` would
then change to match.
##########
core/src/test/java/org/apache/iceberg/TestSchemaParser.java:
##########
@@ -155,4 +156,103 @@ public void
testPrimitiveTypeDefaultValues(Type.PrimitiveType type, Literal<?> d
assertThat(serialized.findField("col_with_default").writeDefault())
.isEqualTo(defaultValue.value());
}
+
+ @Test
+ void requiredUnknownFieldFailsWithFieldName() {
Review Comment:
Two test suggestions: (1) every case here produces at most a single-segment
join (`items.element`, `properties.key`); none nests deeply enough for the
parent path to already be dotted, so segment accumulation across more than one
level is untested. A case such as a struct inside a struct (`a.b.c`) or a list
inside a nested struct (`a.b.element`) would confirm the segments accumulate
rather than overwrite. (2) These four methods are near-identical
JSON-plus-assert blocks, and this file already uses
`@ParameterizedTest`/`MethodSource`, so folding them into one parameterized
test over `(json, expectedMessage)` would remove the duplication.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
