sunchao commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3836790423
##########
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] Apply the empty-struct guard to collect_set too
Could we also reject empty-struct-containing inputs in `CometCollectSet`?
With the `marker: struct<>` LocalRelation from the new tests,
`spark.comet.exec.localTableScan.enabled=true` and native shuffle, `SELECT
collect_set(marker) FROM t` is now admitted to native aggregation.
`SparkCollectSet` wraps DataFusion 54.1's `DistinctArrayAggAccumulator`, which
calls `ScalarValue::compacted()` for each non-null input and hits the same
zero-field `StructArray::new` panic as FIRST/LAST. I reproduced this through
the pinned accumulator with both top-level and nested empty structs, while
Spark 3.5.2 and 4.0.4 return a one-element array. The ordinary Partial/Final
plan has no PartialMerge stage, so the existing collect-buffer fallback does
not contain it. Please reuse the recursive guard for `collect_set` and add a
regression query.
##########
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] Preserve fallback for nested-array empty-struct literals
Could we keep literal fallback until these element types can be serialized?
Over a Parquet-backed table, `SELECT id, array(array(struct())) FROM t` folds
to a non-null `array<array<struct<>>>` literal. The widened predicate changes
`CometLiteral.getSupportLevel` from `Unsupported` to `Compatible`, but
`makeListLiteral` recursively reaches `StructType()` without a matching branch
and throws `scala.MatchError` during planning. I reproduced that base/head
difference using the complete literal serializer, with Spark 4.0.4 successfully
executing the query over an `id: bigint` Parquet scan. The exception is not
caught by the expression or operator conversion path. Please either implement
this serialization or recursively restrict the literal element types, and add a
regression test that keeps constant folding enabled.
--
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]
