Re: [PR] fix: decline Parquet scans whose struct repeats a field id [datafusion-comet]

["andygrove (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org