sunchao commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3836857468
##########
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] Keep empty-struct grouping keys off native aggregation
With `spark.comet.exec.localTableScan.enabled=true`, `SELECT DISTINCT marker
FROM t` for a LocalRelation containing nullable `marker: struct<>` now passes
this gate and retains a native group-only/partial aggregate. Falling back the
complex-key hash exchange does not revert that already-converted child. In
pinned DataFusion 54.1, `GroupValuesRows::emit` calls
`dictionary_encode_if_necessary`, whose struct branch uses
`StructArray::try_new` with zero fields, so emitting the groups fails with
Arrow's `InvalidArgumentError`. I reproduced this through the actual
`AggregateExec` in Partial mode, both with no aggregate functions and with
COUNT; Spark 3.5.2/4.0.4 return the expected empty-struct and NULL groups.
Nested/list-contained empty-struct keys also fail. Please reject these grouping
expressions or fix the group reconstruction before admitting them; the
FIRST/LAST guard does not cover group-key emission.
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -539,7 +539,9 @@ object QueryPlanSerde extends Logging with CometExprShim
with CometTypeShim {
case dt if isTimeType(dt) =>
true
case s: StructType if allowComplex =>
- s.fields.nonEmpty &&
s.fields.map(_.dataType).forall(supportedDataType(_, allowComplex))
+ // 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.
+ s.fields.map(_.dataType).forall(supportedDataType(_, allowComplex))
Review Comment:
[P2] Guard typed NULL array defaults in LEAD/LAG
This also admits `CAST(NULL AS ARRAY<STRUCT<>>)` as a window default. With
native local-table scan/window execution enabled and an `arr: array<struct<>>`
input, `lag(arr, 1, CAST(NULL AS ARRAY<STRUCT<>>)) OVER (ORDER BY id)` (and
`lead`) passes the literal-default checks, but DataFusion 54.1 casts the typed
NULL list to the input type. That recursively casts its zero-length struct
child and errors with `Cannot cast struct with 0 fields to 0 fields because
there is no field name overlap`, even when source and target datatypes are
identical. The new FIRST/LAST guard does not run for these builtin windows.
Spark 3.5.2/4.0.4 preserve the typed NULL and return the expected rows; I
reproduced the failure with the pinned `create_window_expr`, while
omitted/plain NULL defaults and nonempty-struct controls succeed. Please keep
these defaults on Spark or fix their native coercion. This NULL path never
invokes `makeListLiteral`, so fixing the already-reported non-null nested-array
literal do
es not address it.
--
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]
