sunchao commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3875587004
##########
spark/src/main/scala/org/apache/comet/DataTypeSupport.scala:
##########
@@ -54,8 +54,9 @@ trait DataTypeSupport {
CalendarIntervalType =>
true
case StructType(fields) =>
- fields.nonEmpty && fields.forall(f =>
- isTypeSupported(f.dataType, f.name, fallbackReasons))
+ // A struct's `fields` can be empty -- e.g. Iceberg's `_partition`
metadata column is
+ // exactly that on an unpartitioned table. It's still a value Comet
can represent.
+ fields.forall(f => isTypeSupported(f.dataType, f.name,
fallbackReasons))
Review Comment:
[P2] Guard empty-struct array coercion across list field names
For a nonfoldable RDD input `n: struct<e: struct<>>` with nullable `n`/`e`
fields and only non-null values, enabling
`spark.comet.sparkToColumnar.enabled=true` and
`spark.comet.sparkToColumnar.supportedOperatorList=RDDScan` now admits
`array(array_repeat(n, 1).e, array_repeat(n.e, 1))`. The native getter returns
`List(Field("e", Struct([]), true))`, while `array_repeat` uses `Field("item",
...)`. DataFusion `make_array` reconciles those field names, so Comet inserts
`CastExpr`; DataFusion's nested list cast then rejects the identical
empty-struct children for zero field-name overlap. The old `fields.nonEmpty`
gate kept this input on Spark. This is separate from the nullable-parent
finding: both element fields are nullable and every value is non-null. Please
reconcile the container metadata without this recursive cast, or retain
fallback for this path. Source-traced, not runtime-reproduced.
##########
spark/src/main/scala/org/apache/comet/serde/structs.scala:
##########
@@ -250,7 +250,9 @@ object CometJsonToStructs extends
CometCodegenDispatch[JsonToStructs] with Nativ
private def isSupportedSchema(dt: DataType): Boolean = dt match {
case StructType(fields) =>
- fields.nonEmpty && fields.forall(f => isSupportedSchema(f.dataType))
+ // A struct's `fields` can be empty -- e.g. `from_json(col,
'struct<>')`'s target schema.
+ // With no fields to check, this holds vacuously.
+ fields.forall(f => isSupportedSchema(f.dataType))
Review Comment:
[P2] Serialize from_json's nullable result schema
With `spark.comet.expression.JsonToStructs.allowIncompatible=true`, the
public `from_json` API accepts `StructType(Seq(StructField("e",
StructType(Nil), nullable = false)))`. For a nonconstant input column
containing `"{}"`, Spark applies `schema.asNullable` and returns a null `e`.
This change now admits that schema, but serialization still sends `expr.schema`
at line 239. The missing-field branch creates a null empty-struct child, then
the outer `StructArray::new` rejects that null under the original non-nullable
`e` field. Please serialize the nullable result schema, or retain fallback, and
cover missing/null fields with a user-supplied non-nullable schema. The
previous nested-empty constructor-length fix does not address this metadata
mismatch. Source-traced, not runtime-reproduced.
--
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]
