sunchao commented on code in PR #5732:
URL: https://github.com/apache/datafusion-comet/pull/5732#discussion_r3964714022
##########
spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala:
##########
@@ -845,30 +844,29 @@ case class CometScanRule(session: SparkSession)
} else {
val readSchema = scanExec.scan.readSchema()
- // Identify complex type columns that would trigger accessor
creation failures
- val complexColumns = readSchema
- .filter(field => isComplexType(field.dataType))
+ // A struct inside a list/map does not make the container null
check a struct check.
+ val structColumns = readSchema
+ .filter(field => field.dataType.isInstanceOf[StructType])
Review Comment:
### Correctness
[P2] Preserve nullable elements when projecting a required struct field
Could this narrowing also fix the native `GetArrayStructFields` nullability,
or retain fallback for that case? A nullable list `l` can have nullable struct
elements with required integer field `a`. With `l = [NULL]`, `SELECT l.a FROM t
WHERE l IS NOT NULL` should return `[NULL]`. The old complex-column guard kept
this query on Spark, while this gate admits it with native Iceberg enabled and
the default Spark-to-columnar fallback disabled.
Iceberg Java preserves `a` as non-nullable, and the scan serde preserves
that flag. `GetArrayStructFields` then unions the struct-parent nulls into the
extracted child but still uses the original non-nullable field in both its
declared list type and `GenericListArray::new` ([declared
type](https://github.com/apache/datafusion-comet/blob/3700467a3ca8cab7528fbfddcf8351bf7da0a93b/native/spark-expr/src/array_funcs/get_array_struct_fields.rs#L72),
[array
construction](https://github.com/apache/datafusion-comet/blob/3700467a3ca8cab7528fbfddcf8351bf7da0a93b/native/spark-expr/src/array_funcs/get_array_struct_fields.rs#L145)).
Arrow 59.3.0 rejects the now-nullable values and `new` unwraps the error. The
retained filter cannot help because `[NULL]` is a non-null list. Spark
explicitly uses `containsNull || field.nullable` here.
Please align the declared and runtime element nullability and add this
required-child case to the native-scan regression. The new fixture declares `a`
nullable, so it does not cover it. This is verified from the maintained Spark
4.0, Iceberg Java and locked Arrow call chain, not a full query reproduction.
--
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]
