andygrove commented on code in PR #6004:
URL: https://github.com/apache/datafusion-comet/pull/6004#discussion_r4040953162
##########
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:
I confirmed @sunchao's point about top-level fields with a real run rather
than a compile probe, and it does reproduce on this head.
Same construction as your new test in `CometNativeReaderSuite`, but with the
two id-1 fields at the top level instead of inside `s`:
```
message spark_schema {
optional int64 x = 1;
optional int64 y = 1;
}
```
read back with `StructType(Seq(withFieldId("x", 1), withFieldId("y", 1)))`
and `spark.sql.parquet.fieldId.read.enabled=true`:
```
COMET: ROWS=[10,20] (CometNativeScan in the plan)
SPARK: ERROR=Found duplicate field(s) "1": [x, y] in id mapping mode.
```
That is #5801 exactly, one level up. The reason is structural rather than an
oversight in the predicate: `DataTypeSupport.isSchemaSupported` hands
`isTypeSupported` each field's `dataType`, so the root struct's fields are
never compared against each other and this override cannot see them.
Would you consider overriding `isSchemaSupported` here instead and running
`findDuplicateStructFieldIds` once over the whole requested schema? That covers
the root, and it also drops the repeated traversal, since
`findDuplicateStructFieldIds` already recurses while `isTypeSupported`
re-enters it at every nesting level. Could the root case get a test alongside
the nested one?
##########
spark/src/main/scala/org/apache/comet/DataTypeSupport.scala:
##########
@@ -80,11 +80,73 @@ object DataTypeSupport {
val MAP_KEY = "map key"
val MAP_VALUE = "map value"
+ /** Spark's `StructField` metadata key for a Parquet field id. */
+ private val FIELD_ID_METADATA_KEY = "parquet.field.id"
Review Comment:
Comet already uses `ParquetUtils.hasFieldId` and `ParquetUtils.getFieldId`
for this in `serde/operator/package.scala:48-51` and
`QueryPlanSerde.scala:700`. Any reason not to use
`ParquetUtils.FIELD_ID_METADATA_KEY` and `ParquetUtils.hasFieldId` here rather
than redeclaring the key and hand-rolling the metadata read? It would keep the
two from drifting if Spark ever changes it.
##########
spark/src/main/scala/org/apache/comet/DataTypeSupport.scala:
##########
@@ -80,11 +80,73 @@ object DataTypeSupport {
val MAP_KEY = "map key"
val MAP_VALUE = "map value"
+ /** Spark's `StructField` metadata key for a Parquet field id. */
+ private val FIELD_ID_METADATA_KEY = "parquet.field.id"
+
def isComplexType(dt: DataType): Boolean = dt match {
case _: StructType | _: ArrayType | _: MapType => true
case _ => false
}
+ /** True when two of `fields` carry byte-identical names. */
+ def hasDuplicateFieldNames(fields: Array[StructField]): Boolean =
+ fields.map(_.name).distinct.length != fields.length
+
+ /**
+ * Describes the first struct nested anywhere in `dt` whose children repeat
a Parquet field id,
Review Comment:
Small thing about this doc comment. `ParquetReadSupport.matchIdField` raises
when the id a requested field asks for matches more than one field *in the
file*, whereas this checks whether two *requested* fields share an id. Those
are different predicates, and they only coincide under the equal-schema
condition #5801 describes.
That is enough for the gap you are closing, but the comment reads as though
the second follows from the first, and someone will rely on that later. Could
it say which one is implemented and why the narrower check is sufficient here?
--
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]
