comphead commented on code in PR #6004:
URL: https://github.com/apache/datafusion-comet/pull/6004#discussion_r4042592814
##########
spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala:
##########
@@ -1098,6 +1099,16 @@ case class CometScanTypeChecker() extends
DataTypeSupport with CometTypeShim {
dt: DataType,
name: String,
fallbackReasons: ListBuffer[String]): Boolean = {
+ // Under field id matching Spark resolves each requested field to the one
Parquet field
+ // carrying its id, and raises when more than one answers. A requested
struct that repeats an
+ // id cannot be resolved that way, and the native scan reads it
positionally rather than
+ // raising, so hand the read back to Spark and let it report the
ambiguity. See #5801.
+ lazy val duplicateFieldIds =
+ if (CometParquetUtils.readFieldId(SQLConf.get)) {
+ DataTypeSupport.findDuplicateStructFieldIds(dt, name)
Review Comment:
Fixed in 7fe93280, and thank you both — this was a real gap, not a nit.
@andygrove's run at the root is exactly right, and the cause is structural as
he says: `isSchemaSupported` hands `isTypeSupported` each field's *data type*,
so the schema's own field list is never available to any override.
I took a slightly different shape than "run the check once over the whole
requested schema", so flagging it for you to sanity-check. Rather than one
recursive pass, `CometScanTypeChecker` now overrides both entry points and each
does one **shallow** check of exactly one field list:
- `isSchemaSupported` → the schema's own fields (the root case that was
missing)
- `isTypeSupported` → each nested struct, as the trait's existing recursion
reaches it
Same coverage, and every field list is visited exactly once, so it also
removes the repeated subtree traversal @sunchao flagged under Performance.
Tests, both as requested:
- `CometScanRuleSuite` now leads with the unwrapped root schema, alongside a
wrapped struct and a map value, plus distinct-id / one-id-absent / malformed-id
acceptance controls and the flag-disabled control for each.
- `CometNativeReaderSuite` has the flattened fixture as its own test:
`message spark_schema { optional int64 x = 1; optional int64 y = 1; }`, no
key-value metadata, read back with the matching schema. Comet declines and
Spark reports `Found duplicate field(s) "1"`; with field id matching off the
scan stays native.
One more fix while in here: the root reason was rendering doubled, because
the caller already wraps reasons in `s"Unsupported schema ${requiredSchema}:
..."`. Dropped the redundant prefix.
I did consider the deeper fix of adding a field-list hook to
`DataTypeSupport` itself so every checker gets one, and decided against it
here. It needs an `isRoot` flag whose only job is to suppress the
duplicate-*name* check at the root — top-level output attributes legitimately
repeat a name, a self-join gives two `id` columns and Comet matches those
positionally — and that risks silently changing behaviour for the shuffle,
row-conversion and cache checkers in order to benefit one subclass. Happy to do
it if you'd rather.
##########
spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala:
##########
@@ -1098,6 +1099,16 @@ case class CometScanTypeChecker() extends
DataTypeSupport with CometTypeShim {
dt: DataType,
name: String,
fallbackReasons: ListBuffer[String]): Boolean = {
+ // Under field id matching Spark resolves each requested field to the one
Parquet field
+ // carrying its id, and raises when more than one answers. A requested
struct that repeats an
+ // id cannot be resolved that way, and the native scan reads it
positionally rather than
+ // raising, so hand the read back to Spark and let it report the
ambiguity. See #5801.
+ lazy val duplicateFieldIds =
Review Comment:
Fixed in 7fe93280, and thank you both — this was a real gap, not a nit.
@andygrove's run at the root is exactly right, and the cause is structural as
he says: `isSchemaSupported` hands `isTypeSupported` each field's *data type*,
so the schema's own field list is never available to any override.
I took a slightly different shape than "run the check once over the whole
requested schema", so flagging it for you to sanity-check. Rather than one
recursive pass, `CometScanTypeChecker` now overrides both entry points and each
does one **shallow** check of exactly one field list:
- `isSchemaSupported` → the schema's own fields (the root case that was
missing)
- `isTypeSupported` → each nested struct, as the trait's existing recursion
reaches it
Same coverage, and every field list is visited exactly once, so it also
removes the repeated subtree traversal @sunchao flagged under Performance.
Tests, both as requested:
- `CometScanRuleSuite` now leads with the unwrapped root schema, alongside a
wrapped struct and a map value, plus distinct-id / one-id-absent / malformed-id
acceptance controls and the flag-disabled control for each.
- `CometNativeReaderSuite` has the flattened fixture as its own test:
`message spark_schema { optional int64 x = 1; optional int64 y = 1; }`, no
key-value metadata, read back with the matching schema. Comet declines and
Spark reports `Found duplicate field(s) "1"`; with field id matching off the
scan stays native.
One more fix while in here: the root reason was rendering doubled, because
the caller already wraps reasons in `s"Unsupported schema ${requiredSchema}:
..."`. Dropped the redundant prefix.
I did consider the deeper fix of adding a field-list hook to
`DataTypeSupport` itself so every checker gets one, and decided against it
here. It needs an `isRoot` flag whose only job is to suppress the
duplicate-*name* check at the root — top-level output attributes legitimately
repeat a name, a self-join gives two `id` columns and Comet matches those
positionally — and that risks silently changing behaviour for the shuffle,
row-conversion and cache checkers in order to benefit one subclass. Happy to do
it if you'd rather.
--
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]
