sunchao commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3839062981
##########
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] Preserve nullability when extracting empty structs from arrays
With `spark.comet.sparkToColumnar.enabled=true` and
`spark.comet.sparkToColumnar.supportedOperatorList=RDDScan`, this now admits a
nullable row-source `n: struct<e:struct<>>` whose inner `e` field is
non-nullable. For `n = NULL` and `n = Row(Row())`, `array_repeat(n, 1).e`
should return `[NULL]` and `[{}]`. Current serde accepts it, but
`GetArrayStructFields` reuses `e`'s non-nullable Arrow field for the result
list and panics because the null parent produces a null element. I reproduced
`Non-nullable field of ListArray "e" cannot contain nulls` with the exact
native code using IPC emitted by Comet's actual `RowArrowReader`; Spark 4.0.4
preserves this RDD schema and returns the expected values. Please propagate
array-element nullability to the extracted field or retain fallback for this
case. Local-table-scan tests do not cover it because that source widens nested
nullability.
##########
spark/src/main/scala/org/apache/comet/serde/aggregates.scala:
##########
@@ -837,12 +843,20 @@ object CometCollectSet extends
CometAggregateExpressionSerde[CollectSet] {
" `spark.comet.expression.CollectSet.allowIncompatible=true` is set.")
override def getSupportLevel(expr: CollectSet): SupportLevel = {
- // The native path always drops null inputs. Spark 4.2 added an
`ignoreNulls` field to
- // CollectSet that `RESPECT NULLS` sets to false, preserving nulls in the
result; Comet
- // cannot match that, so fall back. This branch is only reachable on Spark
4.2+: on 3.4
- // through 4.1 the field does not exist, `RESPECT NULLS`/`IGNORE NULLS`
are rejected at
- // analysis time, and CometCollectShim.ignoreNulls hardcodes true, making
this a no-op.
- if (!CometCollectShim.ignoreNulls(expr)) {
+ // DataFusion's DistinctArrayAggAccumulator (backing SparkCollectSet) calls
+ // ScalarValue::compacted() per non-null input, hitting the same zero-field
+ // StructArray::new panic as First/Last -- see
SupportLevel.containsEmptyStruct.
+ if (SupportLevel.containsEmptyStruct(expr.dataType)) {
Review Comment:
[P2] Guard collect_list's nullable-field coercion for empty structs
For `g: INT, marker: STRUCT<>`, `SELECT g,
collect_list(named_struct('marker', marker, 'n', 1)) FROM t GROUP BY g` is
still admitted with native local-table scan enabled. The literal makes `n`
non-nullable, so `coerce_collect_child_nullability` wraps the argument in
DataFusion's `CastExpr`; recursively casting `marker` then fails with `Cannot
cast struct with 0 fields to 0 fields because there is no field name overlap`.
I verified current aggregate-serde admission and reproduced this with the exact
native `CreateNamedStruct`/nullable-type helper and pinned DataFusion 54.1,
while Spark 4.0.4 succeeds. The integer grouping key avoids the grouping guard,
and this CollectSet-only check does not protect CollectList. The previous
schema gates kept this input on Spark. Please also guard the collect-list path
or normalize nullability without casting the empty struct.
--
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]
