vinishjail97 commented on code in PR #826:
URL: https://github.com/apache/incubator-xtable/pull/826#discussion_r3337263082
##########
xtable-core/src/main/java/org/apache/xtable/kernel/DeltaKernelSchemaExtractor.java:
##########
@@ -192,7 +192,7 @@ private InternalSchema toInternalSchema(
toInternalSchema(
Review Comment:
This map-key recursion passes `null` for `originalMetadata`. If a map key is
a `BinaryType`, the binary branch above
(`originalMetadata.contains(InternalSchema.XTABLE_LOGICAL_TYPE)`) has no null
guard and will NPE. The Spark sibling `DeltaSchemaExtractor` guards
`originalMetadata != null` before `.contains(...)`. Since this PR specifically
enables nested/complex map-key handling, a binary-typed map key (or binary
array element — also passes null) now becomes more reachable. Worth adding the
same null guard here?
##########
xtable-core/src/main/java/org/apache/xtable/kernel/DeltaKernelSchemaExtractor.java:
##########
@@ -219,6 +219,8 @@ private InternalSchema toInternalSchema(
type = InternalType.MAP;
fields = Arrays.asList(keyField, valueField);
trimmedTypeName = "map";
+ } else {
Review Comment:
Good — the `else throw NotSupportedException` is required now that
`type`/`trimmedTypeName` are method locals (definite-assignment). One thing to
double-check: were any DataTypes previously falling through to the old `null`
instance-field defaults and silently producing a schema with
`name=""`/`type=null` (e.g. `VariantType`, `NullType`)? If so, this now
correctly fails fast rather than emitting a malformed schema — just confirm
that's the intended behavior change.
--
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]
