comphead commented on code in PR #6004:
URL: https://github.com/apache/datafusion-comet/pull/6004#discussion_r4042596290
##########
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:
Good catch, and you're right that the comment implied the second followed
from the first. Rewritten in 7fe93280 to lead with the distinction:
```
* True when two of `fields` declare the same Parquet field id.
*
* Deliberately not Spark's check: `ParquetReadSupport.matchIdField` raises
when one *requested*
* id is carried by several fields *in the file*. The two coincide only when
the requested
* schema equals the file schema, which is exactly when DataFusion's opener
skips the expression
* adapter that would have validated the lookup (#5801). So this can decline
a read Spark would
* have accepted, costing native execution but not correctness; it cannot
report a duplicate
* Spark would not. File-side ambiguity is left to #5786.
*
* Only meaningful under `spark.sql.parquet.fieldId.read.enabled`; callers
gate on that.
```
It now says which predicate is implemented, why the narrower one is
sufficient here, and which direction the error can go — it can cost native
execution but not correctness, and it cannot invent a duplicate Spark would not
report.
##########
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:
No good reason — done in 7fe93280. `fieldId` now uses
`ParquetUtils.hasFieldId` and `ParquetUtils.getFieldId`, and the redeclared
`FIELD_ID_METADATA_KEY` is gone.
The only wrinkle is that `getFieldId` raises on a non-integral or
out-of-range id, and this runs during planning where raising would be the wrong
error, so it is wrapped:
```scala
private def fieldId(field: StructField): Option[Int] = {
if (!ParquetUtils.hasFieldId(field)) None
else {
// A malformed id is not this check's business to report -- getFieldId
raises on one -- so
// treat it as absent and let the reader complain about it.
try Some(ParquetUtils.getFieldId(field))
catch { case _: IllegalArgumentException => None }
}
}
```
Still Spark's own definition of what a field id is, which is the drift you
were guarding against. There's a `malformed id` case in the
`CometScanRuleSuite` acceptance controls pinning that it is not misreported as
a duplicate.
Related, from the same cleanup: `hasDuplicateFieldNames` was extracted to be
shared, but two inline copies of `fields.map(f => f.name).distinct.length ==
fields.length` were still sitting in `CometShuffleExchangeExec` at lines 483
and 610. Those now call the shared predicate. I left
`CometBatchKernelCodegen:103` alone — it works off `st.fieldNames` with a
deliberate comment about not rebuilding that array, and converting it would
reintroduce the allocation the comment avoids.
--
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]
