Skip to content

fix: support empty struct types - #5414

Closed
unikdahal wants to merge 1 commit into
apache:mainfrom
unikdahal:fix-empty-struct-shuffle-support
Closed

fix: support empty struct types#5414
unikdahal wants to merge 1 commit into
apache:mainfrom
unikdahal:fix-empty-struct-shuffle-support

Conversation

@unikdahal

@unikdahal unikdahal commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #5413.

Rationale for this change

Six copies of the same guard reject a zero-field StructType, treating it the same
as a genuinely unsupported type. It's a legitimate Arrow value. Iceberg's _partition
metadata column is exactly this shape on an unpartitioned table, so any plan carrying
it silently fell back to Spark at the first shuffle/sink/scan boundary.

What changes are included in this PR?

Removed the empty-struct exclusion from all six checks: native shuffle, columnar
shuffle, CometSink, QueryPlanSerde.supportedDataType, DataTypeSupport, and
from_json's target-schema check. The last one alone wasn't safe to fix Scala-side --
it uncovered a real native panic in from_json.rs (StructArray::new can't derive row
count with zero child arrays), fixed by branching to StructArray::new_empty_fields.

How are these changes tested?

Added new tests to test the fix.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR treats zero-field structs as supported values across Comet's schema gates, adds native shuffle coverage for empty structs, and teaches native from_json to construct a top-level zero-field Arrow StructArray with an explicit row count.

Prior state and problem

Several independent support checks rejected StructType when it had no fields, causing otherwise representable schemas—such as Iceberg's empty _partition metadata struct for unpartitioned tables—to fall back at shuffle, sink, or serialization boundaries. Native from_json also used StructArray::new, whose length cannot be inferred when the result has no child arrays.

Design approach

The patch removes the non-empty requirement from the Scala support predicates and adds a top-level Rust branch that calls StructArray::new_empty_fields(num_rows, ...). The new shuffle tests exercise empty structs through native and columnar exchanges, while the JSON test covers a top-level struct<> target schema with and without dictionary encoding.

Correctness / compatibility analysis

The top-level empty-struct construction is consistent with Arrow 58.4.0 and preserves the input row count and validity buffer. The general support-check changes are also structurally consistent for empty structs. One native from_json gap remains, however: the recursive Scala predicate also enables nested empty structs, while the nested Rust builder still uses StructArray::new with zero child arrays and therefore panics.

Key design decisions

The patch correctly distinguishes an empty struct from an unsupported data type, preserves recursive validation for non-empty children, and limits the specialized Arrow constructor to the case where length cannot be inferred. The remaining decision is whether nested empty structs should be supported now or kept off the native from_json path until their builder uses the same explicit-length construction.

Implementation sketch

Six Scala guards are relaxed, the top-level JSON result chooses between new_empty_fields and StructArray::new, and focused shuffle/JSON tests are added. The nested FieldBuilder::Struct finalization path is unchanged.

Behavioral changes worth calling out

Plans carrying zero-field structs can remain in Comet through the updated boundaries instead of falling back. With native from_json opt-in enabled, top-level struct<> now works, but a schema such as struct<outer:struct<>> is also accepted and reaches an Arrow constructor that panics.

Suggested improvements

Handle zero-field nested structs in finish_builder using the nested validity-buffer length and add a regression test for a nested-empty schema. Alternatively, keep nested empty structs unsupported by the native JSON predicate until that construction path is implemented.

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))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Keep nested empty structs off the native path until they are constructed safely

Because this predicate is recursive, it now admits struct<outer:struct<>>, not just the top-level struct<> covered by the new test. The top-level Rust branch does not cover that shape: finish_builder still calls StructArray::new for every nested FieldBuilder::Struct; for an empty nested struct, builders yields zero child arrays. Arrow 58.4.0's StructArray::new unwraps try_new, which rejects no child arrays, so the query panics once this PR routes it native. Please add the same empty-fields construction in finish_builder (using its null-buffer length) and a nested-empty regression test, or keep nested empties unsupported here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick, thorough review @sunchao .

Good catch, the recursive Scala predicate let struct<outer:struct<>> through while
finish_builder's nested struct branch still panicked on it. Fixed with the same
is_empty() check, using null_buf.len() for the row count. Added a regression test
covering the nested case. All 10 tests in CometJsonExpressionSuite pass.

// `fields` is empty (a legitimate zero-field target schema, e.g. `from_json(_, 'struct<>')`).
// `new_empty_fields` takes the length explicitly instead.
let struct_array: ArrayRef = if fields.is_empty() {
Arc::new(StructArray::new_empty_fields(num_rows, Some(null_buffer)))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve Spark NULLs for blank JSON with an empty struct schema

With spark.comet.expression.JsonToStructs.allowIncompatible=true, this branch now makes from_json(col, 'struct<>') execute natively when col is '' or whitespace. The native parser marks every parse error as a valid struct, so the exact-head expression produces a non-null Row() and from_json(col, 'struct<>') IS NULL is false; Spark 3.5 and 4.0 intentionally return NULL for blank inputs (SPARK-19543). Before this change the empty schema took the Spark/codegen path, so this is a newly introduced wrong-result case. The added test uses only {}; please preserve the NULL validity bit for blank input and add empty/whitespace regression rows.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blank/whitespace input now short-circuits to NULL before parsing, matching SPARK-19543, separate from non-blank malformed input (still PERMISSIVE null-fields). New regression rows cover blank, whitespace, non-blank-malformed, and SQL NULL, checked against real Spark output.

Comment thread native/spark-expr/src/json_funcs/from_json.rs Outdated
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))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Guard empty structs in FIRST_VALUE/LAST_VALUE windows

This also admits empty-struct inputs to native windows that cannot handle them. Using the marker: struct<> LocalRelation from the new tests, enable spark.comet.exec.localTableScan.enabled=true with native shuffle and run SELECT first_value(marker) OVER () FROM t (no ORDER BY). The input can now stay native through CometWindowExec, which maps this to DataFusion 54.1's FirstValue. Its accumulator calls ScalarValue::compact(), whose compact_view_buffers struct branch reconstructs the array with StructArray::new even when there are no child fields, causing an Arrow panic. I reproduced this through the dependency's WindowExpr::evaluate on a valid three-row empty-struct batch; Spark returns three empty structs. LAST_VALUE fails identically, and the compaction is recursive, so nested/list/map-contained empty structs also fail. The old type gates kept these inputs on Spark. Please keep these windows on Spark for schemas containing empty structs until scalar compaction is fixed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. CometFirst/CometLast now decline any schema containing an empty struct
(recursively through struct/array/map, matching where ScalarValue::compact would panic)
via a getSupportLevel check applies whether First/Last is used as a plain aggregate
or a window function, since both share the same serde. Added a regression test
reproducing your exact repro; confirmed no panic, correct fallback.

Also opened the actual fix upstream: apache/datafusion#24582.

for (field, builder) in fields.iter().zip(field_builders.iter_mut()) {
let field_value = obj.get(field.name());
append_field_value(builder, field, field_value)?;
if json_str.trim().is_empty() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Restrict blank-input detection to JSON whitespace

With spark.comet.expression.JsonToStructs.allowIncompatible=true, trim() also treats non-JSON whitespace such as NBSP (U+00A0), vertical tab and form feed as a blank document. For a column containing only NBSP, the exact-head native expression now makes from_json(col, 'struct<>') IS NULL true, while Spark 3.5.2 and 4.0.4 return a non-null empty struct. This also regresses already-supported schemas such as a INT: both the base and previous native head return the non-null, all-null-fields struct, but this new branch returns SQL NULL. Restrict the blank check to JSON whitespace (space, tab, CR and LF) and add a non-JSON-whitespace regression row alongside the ordinary blank-input cases.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Restricted the blank check to JSON whitespace (space/tab/CR/LF), verified against
Jackson's actual tokenizer behavior in current Spark source, not just the old SPARK-19543
comment. Added an NBSP regression row alongside the ordinary blank-input cases.

@unikdahal
unikdahal requested a review from sunchao August 22, 2026 18:36

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up on 34083c79d1ed: two remaining empty-struct regressions.

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))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

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))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

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))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Rejects grouping keys containing an empty struct in both CometBaseAggregate.doConvert and the mirrored canAggregateBeConverted tag check (per its own WARNING comment). Added regression tests for GROUP BY and DISTINCT.

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))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 does not address it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Declines LAG/LEAD when the default expression's own type carries an empty struct - keyed on that, not the input type, so omitted/plain-NULL defaults (which don't hit the cast) stay native. Added regression tests for both the failing and the still-native forms.

@unikdahal
unikdahal requested a review from sunchao August 22, 2026 21:54
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))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Guard nested-empty values in native scalar-ordering paths

With native local-table scan/shuffle enabled, let df contain id: INT and a: ARRAY<STRUCT<marker:STRUCT<>>>, with a = [Row(null), Row(Row())]. df.repartition(2, df("id")).selectExpr("array_max(a)") stays on Spark at the pinned base, but the exact-head planner converts it to CometProject -> CometNativeShuffle -> CometLocalTableScan. Spark 3.5.2/4.0.4 returns the element whose marker is {}; the pinned DataFusion 54.1 UDF instead returns the element whose marker is NULL.

ScalarValue::partial_cmp_struct recursively flattens structs, so the zero-field child contributes neither fields nor validity and those two values compare equal. The same comparator also misidentifies RANGE-window peers: with s: struct<e:struct<>> and a second integer order key, COUNT(*) OVER (ORDER BY s, k RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) counts {e:NULL} and {e:{}} together. The new compaction, grouping-reconstruction, and default-cast guards do not cover these ordering paths. Please keep the affected array extrema and RANGE order keys containing empty structs on Spark until native comparison preserves nested validity.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed both. Same root cause, two spots: ScalarValue::partial_cmp_struct flattens a struct to its leaf columns to compare, so a zero-field struct loses its own validity bit and a NULL element ties with a non-null {} element. Guarded array_max/array_min on element type, and RANGE frame ordering on the ORDER BY key type (the existing offset checks only covered explicit-offset bounds UNBOUNDED/CURRENT ROW skipped them entirely). Also checked sort_array and plain ORDER BY both use arrow's row-format comparator instead of ScalarValue::partial_cmp, which encodes struct-level validity independent of field count, so they're not affected by this one

@unikdahal
unikdahal requested a review from sunchao August 23, 2026 09:22
// 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)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

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))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@unikdahal
unikdahal force-pushed the fix-empty-struct-shuffle-support branch 2 times, most recently from a4f1ce1 to ec46f53 Compare August 27, 2026 19:21

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] At ec46f533, the new guard addresses the collect_list finding. The existing extraction issue remains: the helper widens the runtime list field, but data_type() still declares the original non-nullable field, so the nullable-parent RDD case fails projection schema validation.

The two additional findings are in the inline comments. These are current-source/dependency checks, not runtime reproductions; no tests or builds were run, and the three current workflows remain action_required.

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))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

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))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@unikdahal
unikdahal force-pushed the fix-empty-struct-shuffle-support branch from ec46f53 to 62f3a34 Compare August 27, 2026 21:21
@unikdahal
unikdahal force-pushed the fix-empty-struct-shuffle-support branch from 62f3a34 to eb13d5e Compare August 27, 2026 22:09
@unikdahal

Copy link
Copy Markdown
Contributor Author

@sunchao Thanks again for the detailed review. I went through the remaining empty-struct paths and pushed another round of fixes.

The latest changes address the outstanding coercion/schema issues:

  • GetArrayStructFields now derives the output field consistently for both data_type() and evaluate(), so parent-null propagation cannot produce runtime data that disagrees with the declared Arrow type. The regression also validates the produced array against a RecordBatch.
  • from_json now serializes expr.dataType instead of the user-provided schema, matching Spark's schema.asNullable result contract.
  • multi-argument CreateArray with an empty struct in the element type now falls back before DataFusion can insert a problematic zero-field struct cast.
  • greatest / least now conservatively fall back for multi-argument inputs containing an empty struct. This also covers Arrow-only container metadata differences such as list<e: struct<>> vs list<item: struct<>>, which Spark's DataType does not expose.
  • map lookups (m[key] / element_at(m, key)) now fall back when the map key type contains an empty struct, since DataFusion's map_extract coerces the lookup key to the map's exact Arrow key type and can otherwise hit the same zero-field cast failure.
  • added/expanded regressions for the above, including the Arrow field-name mismatch case and NULL vs {} validity through shuffle.

Would appreciate another look when you get a chance.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked eb13d5e. Two existing P2 findings still have residual cases with a struct-only RDD schema (id: INT, nullable n: struct<e:struct<>>, nullable e), spark.comet.sparkToColumnar.enabled=true, and RDDScan in the supported-operator list:

  • [P2] Extraction: with (id,n) = (1,{e:NULL}), (2,{e:{}}) in one batch, array_repeat(IF(id=1,n,NULL),1).e produces [NULL], [{}] instead of two [NULL] arrays. Equal null counts skip merging different parent/child validity masks. The earlier declared-type mismatch is fixed.
  • [P2] List coercion: with both n values {e:{}} and ids 1 and 2 in one batch, IF(id=1,array_repeat(n,1).e,array_repeat(n.e,1)) hits the zero-field cast error because the branches have different Arrow element names. The original CreateArray case is now guarded.

Both residuals were reproduced with pinned native components, including the registered SparkArrayRepeat. Spark admission and optimizer paths were source-checked; these were not full Spark/JNI query runs. The two new inlines concern separate regression-test failures seen in current merge-checkout CI.

Comment on lines +65 to +67
checkSparkAnswerAndFallbackReason(
"SELECT outer <=> named_struct('e', struct()) FROM empty_struct_cmp",
"on differently-typed operands containing an empty struct is not supported")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Make this fixture exercise the comparison guard

This assertion fails in all six current expression CI jobs: the actual reason is Unsupported data type StructType(StructField(e,StructType(),true)), rather than the comparison-mismatch reason. The checked Spark 3.5 coercion path merges nested nullability and folds the named_struct operand into a non-null struct literal, which Comet rejects earlier. The fixture therefore blocks the expression test jobs without validating its stated guard. Verify the analyzed operand types or use a focused serde test with genuinely differing types; changing only the expected string would stop testing the intended guard. Current CI example.

spark.read.parquet(path.toString).createOrReplaceTempView("t1")
checkSparkAnswerAndFallbackReason(
"SELECT id, array(array(struct())) FROM t1",
"Unsupported data type array<array<struct")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Match the literal fallback's actual type rendering

The fallback helper performs case-sensitive substring matching, but CometLiteral.getSupportLevel interpolates the DataType object and emits Unsupported data type ArrayType(ArrayType(StructType(),false),false), not the lowercase catalog syntax asserted here. The fallback and result comparison succeed, then this new assertion fails in all six current expression CI jobs. Match the serializer's actual diagnostic contract, or deliberately standardize that diagnostic and its expectations. Current CI example.

@andygrove andygrove added enhancement New feature or request area:expressions Expression evaluation labels Sep 6, 2026

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment you add to supportedSerializableDataType says an empty struct serializes through native shuffle like any other struct, but supportedHashPartitioningDataType a few lines up at CometShuffleExchangeExec.scala:445 still has fields.nonEmpty, and its comment justifies that with "an in-memory relation with one does not survive scan conversion". That gate landed on main in #5567 on 2026-09-06, after this branch point, so it is not in your diff. After this PR the claim is no longer true, because CometLocalTableScanExec.isTypeSupported and CometSparkToColumnarExec.isTypeSupported both fall through to super for structs and will now accept a struct<>. Could you rebase and reconcile the two, so serialization and hash partitioning agree and the stale justification goes away?

The other thing I would like settled is CometCreateNamedStruct. Its getSupportLevel at spark/src/main/scala/org/apache/comet/serde/structs.scala:41 only rejects duplicate field names and never looks at the data type, and the native side at native/spark-expr/src/struct_funcs/create_named_struct.rs:85 calls StructArray::new, which unwraps try_new, and try_new errors outright when there are no child arrays because it cannot infer a length. The !values.is_empty() guard on all_scalar just above means a zero-child call takes exactly that branch. struct() normally constant-folds to a literal, which is why your array(array(struct())) and named_struct('e', struct()) cases pass, but with ConstantFolding in spark.sql.optimizer.excludedRules it would not fold. Have you checked that path? You fixed the same shape of problem in from_json.rs in this PR, so it seems worth covering here as well.

docs/source/user-guide/latest/datatypes.md:100 still says empty structs fall back. That page is hand maintained and GenerateDocs does not regenerate it, so it needs an edit here. The accurate wording is a little fiddly, since after this PR they do still fall back for Parquet and Iceberg data columns through the s.fields.isEmpty guard in CometScanTypeChecker at CometScanRule.scala:1116.

CI is green on the current head.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness

Rechecked eb13d5e4275556df29c5c9ba70a3a64f0f33c1e3 against assigned base eabb5d4773091b983d8fce713f0e34b1cf93f877 after the new discussion. Both pins are unchanged from my previous review. The four existing P2 findings remain unresolved. This follow-up adds no duplicate inline comments.

  • [P2] Parent-null propagation: the extraction finding still has the residual described in that review. The shared output-field calculation fixes declared nullability, but get_array_struct_fields still skips the validity union whenever parent and child have equal null counts. Different masks can have the same count, so a child {} can survive under a null parent. The added native tests cover differing counts, not this case.
  • [P2] Conditional list coercion: the list-field-name finding still applies to IF. CometIf has no equivalent of the new CreateArray guard. Its native CaseExpr casts the else branch to the then branch's Arrow type. list<item: struct<>> versus list<e: struct<>> reaches DataFusion's recursive struct cast, which rejects a non-null zero-field struct. The prior review's component results were not rerun here. I rechecked the unchanged Comet code and the checksum-verified DataFusion 54.1.0/Arrow 58.4.0 sources.

[P2] CI is still red. All six expression jobs in run 33121358952 fail the already reported comparison-fixture assertion and nested-array-literal reason assertion. The comparison actually falls back on an unsupported folded struct literal, and the array reason is rendered as ArrayType(ArrayType(StructType(),false),false). The other five failures are also actionable: four Scalafix jobs require removing the unused import at CometMapExpressionSuite.scala:284, and the Rust job fails Clippy because the new test module precedes the Display implementation. Cargo tests are skipped. All eleven logs check out historical merge 71c96b1. Its 25 PR-authored file blobs match this head. These are historical-merge results, not validation against later main.

The zero-child constructor concern in the new review is valid as a source-level support gap: StructArray::new cannot construct zero children, and the serde accepts zero names. However, that constructor and acceptance path already exist in the assigned base. The cited all_scalar branch belongs to later main, not this head. A normally folded struct() also follows the unsupported non-null literal path. Could you record this pre-existing constructor gap in a tracking issue and link it here? I am not presenting it as a newly introduced regression.

The source comparison used the maintained Spark 3.5 and 4.0 branches. The required Spark 3.4 and 4.1 branches were unavailable, so I did not perform a fresh canonical-source audit for those versions.

Performance

There is no new performance implementation since the previous review. The recursive empty-struct checks operate on schemas during conversion, and the new conservative guards reduce native coverage for the affected operations. I found no additional performance finding from this follow-up. No local benchmark or end-to-end runtime test was run, so this does not establish an acceleration benefit.

Design

The shuffle observation needs an integration distinction. PR #5567 added nested hash keys after this PR's assigned base. That base/head reject complex hash keys generally. At inspected later-main commit 5627ab8, the new hash-key gate still requires fields.nonEmpty. When integrating this PR, reconcile its comment that in-memory empty structs cannot survive scan conversion: this PR admits them through the local-table and eligible Spark-to-columnar paths. Keeping a separate hash-key restriction can be appropriate, but the reason and tests should reflect the actual boundary. The new shuffle tests repartition by integer id, so they prove no empty-struct hash-key support.

Abstraction & complexity

The shared recursive predicate and output-field helper remain sensible places for the schema policy. The residual IF cast shows why capability checks must follow each coercion path, including Arrow field-name differences invisible to Spark's ArrayType. The hand-maintained datatypes.md statement that all empty structs fall back should also be narrowed: this PR admits carrying them, while operation-specific fallbacks and CometScanTypeChecker's empty data-column restriction remain. Iceberg metadata columns are explicitly exempted from that scan data-schema check. This distinction is more accurate than declaring unconditional empty-struct support.

@unikdahal

Copy link
Copy Markdown
Contributor Author

Thanks @sunchao and @andygrove for the review here.

Given how many additional empty-struct edge cases this uncovered across DataFusion-backed paths, I think it makes more sense to stop growing the set of Comet-side guards/workarounds in this PR and revisit this from a cleaner dependency baseline.

The upstream ScalarValue::compact / new_default fix has now been backported to branch-55 in apache/datafusion#24876 for 55.1.0. That addresses one important class of failures we hit here, but the review also surfaced several other paths that should be re-evaluated independently rather than assuming empty structs are universally safe.

I'm going to close this PR for now and pick it back up once Comet is on DataFusion 55.1. I'll re-audit the remaining cases from that baseline and keep the Comet-side changes as narrow as possible.

Thanks again for digging into all of these cases.

@unikdahal unikdahal closed this Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:expressions Expression evaluation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Empty struct columns silently fall back to Spark instead of running natively

3 participants