You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "unikdahal (via GitHub)" <gi...@apache.org> on 2026/08/22 13:56:35 UTC
[PR] fix: support empty struct types [datafusion-comet]
unikdahal opened a new pull request, #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414
## 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?
New general-purpose tests (no Iceberg involved): native/columnar shuffle on an
empty-struct column, and `from_json(..., 'struct<>')`. Also ran `CometNativeShuffleSuite`,
`CometShuffleSuite`, `CometExecSuite`, `CometJsonExpressionSuite` -- all pass, including
the existing tests that assert legitimate cases still fall back.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3836225436
##########
spark/src/main/scala/org/apache/comet/serde/structs.scala:
##########
@@ -250,7 +250,9 @@ object CometJsonToStructs extends CometCodegenDispatch[JsonToStructs] with Nativ
private def isSupportedSchema(dt: DataType): Boolean = dt match {
case StructType(fields) =>
- 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))
Review Comment:
[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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3875587004
##########
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] 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.
##########
spark/src/main/scala/org/apache/comet/serde/structs.scala:
##########
@@ -250,7 +250,9 @@ object CometJsonToStructs extends CometCodegenDispatch[JsonToStructs] with Nativ
private def isSupportedSchema(dt: DataType): Boolean = dt match {
case StructType(fields) =>
- 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))
Review Comment:
[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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "unikdahal (via GitHub)" <gi...@apache.org>.
unikdahal commented on PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#issuecomment-5606105798
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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "unikdahal (via GitHub)" <gi...@apache.org>.
unikdahal commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3836280849
##########
spark/src/main/scala/org/apache/comet/serde/structs.scala:
##########
@@ -250,7 +250,9 @@ object CometJsonToStructs extends CometCodegenDispatch[JsonToStructs] with Nativ
private def isSupportedSchema(dt: DataType): Boolean = dt match {
case StructType(fields) =>
- 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))
Review Comment:
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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "unikdahal (via GitHub)" <gi...@apache.org>.
unikdahal commented on PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#issuecomment-5445817975
@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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "unikdahal (via GitHub)" <gi...@apache.org>.
unikdahal commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3836435855
##########
native/spark-expr/src/json_funcs/from_json.rs:
##########
@@ -393,7 +397,14 @@ fn finish_builder(builder: FieldBuilder) -> Result<ArrayRef> {
.map(finish_builder)
.collect::<Result<Vec<_>>>()?;
let null_buf = arrow::buffer::NullBuffer::from(null_buffer);
- Arc::new(StructArray::new(fields, nested_arrays, Some(null_buf)))
+ // `StructArray::new` derives its row count from the first child array; a zero-field
+ // schema (e.g. a `struct<>`-typed field nested inside a larger schema) has no child
+ // arrays to derive it from, so the count is supplied explicitly here instead.
+ if fields.is_empty() {
+ Arc::new(StructArray::new_empty_fields(null_buf.len(), Some(null_buf)))
Review Comment:
Apologies, missed running `cargo fmt` before pushing. Fixed. You can trigger the CI run now @sunchao
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "unikdahal (via GitHub)" <gi...@apache.org>.
unikdahal commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3836729220
##########
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:
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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "unikdahal (via GitHub)" <gi...@apache.org>.
unikdahal commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3836434349
##########
native/spark-expr/src/json_funcs/from_json.rs:
##########
@@ -180,11 +180,15 @@ fn json_string_to_struct(arr: &Arc<dyn Array>, schema: &DataType) -> Result<Arra
.map(finish_builder)
.collect::<Result<Vec<_>>>()?;
let null_buffer = NullBuffer::from(struct_nulls);
- Ok(Arc::new(StructArray::new(
- fields.clone(),
- arrays,
- Some(null_buffer),
- )))
+ // `StructArray::new` derives its length from the first child array, so it panics when
+ // `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)))
Review Comment:
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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "unikdahal (via GitHub)" <gi...@apache.org>.
unikdahal commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3837101778
##########
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:
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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "unikdahal (via GitHub)" <gi...@apache.org>.
unikdahal commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3836727656
##########
native/spark-expr/src/json_funcs/from_json.rs:
##########
@@ -150,27 +150,35 @@ fn json_string_to_struct(arr: &Arc<dyn Array>, schema: &DataType) -> Result<Arra
} else {
let json_str = string_array.value(row_idx);
- // Parse JSON (PERMISSIVE mode: return null fields on error)
- match serde_json::from_str::<serde_json::Value>(json_str) {
- Ok(json_value) => {
- if let serde_json::Value::Object(obj) = json_value {
- // Struct is not null, extract each field
- *struct_null = true;
- 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() {
Review Comment:
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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3878071865
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -44,6 +44,100 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
val DIVIDE_BY_ZERO_EXCEPTION_MSG =
"""Division by zero. Use `try_divide` to tolerate divisor being 0 and return NULL instead"""
+ test("struct comparison declines mismatched-nullability operands containing an empty struct") {
+ // planner.rs's reconcile_nested_comparison_types casts whichever comparison operand doesn't
+ // already match the nullability-union of both operand types; a named_struct literal argument
+ // infers a non-nullable field, so comparing it against a schema-nullable column of the same
+ // shape casts one side. DataFusion casts a struct field-by-field even when only that one
+ // field's nullability differs, and casting a zero-field struct to itself still fails -- see
+ // SupportLevel.containsEmptyStruct.
+ withSQLConf(
+ CometConf.COMET_EXEC_LOCAL_TABLE_SCAN_ENABLED.key -> "true",
+ SQLConf.OPTIMIZER_EXCLUDED_RULES.key ->
+ "org.apache.spark.sql.catalyst.optimizer.ConvertToLocalRelation") {
+ val schema =
+ StructType(Seq(StructField("outer", StructType(Seq(StructField("e", StructType(Nil)))))))
+ val data =
+ java.util.List.of[Row](Row(Row(Row())), Row(Row(null)), Row(null))
+ val df = spark.createDataFrame(data, schema)
+ df.createOrReplaceTempView("empty_struct_cmp")
+
+ checkSparkAnswerAndFallbackReason(
+ "SELECT outer <=> named_struct('e', struct()) FROM empty_struct_cmp",
+ "on differently-typed operands containing an empty struct is not supported")
Review Comment:
[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](https://github.com/apache/datafusion-comet/actions/runs/33121358952/job/98722616148).
##########
spark/src/test/scala/org/apache/comet/CometArrayExpressionSuite.scala:
##########
@@ -890,6 +891,73 @@ class CometArrayExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelp
}
}
+ test("array literal of empty struct falls back instead of crashing planning") {
+ // `array(array(struct()))` constant-folds to a literal whose (doubly-nested) array element
+ // type is an empty struct. `makeListLiteral` has no case for StructType at all (empty or
+ // not) -- letting this reach the literal path throws a bare `scala.MatchError` at plan
+ // time rather than a graceful fallback. See CometLiteral.isListLiteralElementSupported.
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "test.parquet")
+ spark.range(10).write.parquet(path.toString)
+ withTempView("t1") {
+ spark.read.parquet(path.toString).createOrReplaceTempView("t1")
+ checkSparkAnswerAndFallbackReason(
+ "SELECT id, array(array(struct())) FROM t1",
+ "Unsupported data type array<array<struct")
Review Comment:
[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](https://github.com/apache/datafusion-comet/actions/runs/33121358952/job/98722616148).
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "unikdahal (via GitHub)" <gi...@apache.org>.
unikdahal commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3837100686
##########
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:
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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3836344840
##########
native/spark-expr/src/json_funcs/from_json.rs:
##########
@@ -180,11 +180,15 @@ fn json_string_to_struct(arr: &Arc<dyn Array>, schema: &DataType) -> Result<Arra
.map(finish_builder)
.collect::<Result<Vec<_>>>()?;
let null_buffer = NullBuffer::from(struct_nulls);
- Ok(Arc::new(StructArray::new(
- fields.clone(),
- arrays,
- Some(null_buffer),
- )))
+ // `StructArray::new` derives its length from the first child array, so it panics when
+ // `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)))
Review Comment:
[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.
##########
native/spark-expr/src/json_funcs/from_json.rs:
##########
@@ -393,7 +397,14 @@ fn finish_builder(builder: FieldBuilder) -> Result<ArrayRef> {
.map(finish_builder)
.collect::<Result<Vec<_>>>()?;
let null_buf = arrow::buffer::NullBuffer::from(null_buffer);
- Arc::new(StructArray::new(fields, nested_arrays, Some(null_buf)))
+ // `StructArray::new` derives its row count from the first child array; a zero-field
+ // schema (e.g. a `struct<>`-typed field nested inside a larger schema) has no child
+ // arrays to derive it from, so the count is supplied explicitly here instead.
+ if fields.is_empty() {
+ Arc::new(StructArray::new_empty_fields(null_buf.len(), Some(null_buf)))
Review Comment:
[P2] Format this nested constructor so required CI can run
`cargo fmt --all -- --check` now exits 1 on this line; both the pinned base and previously reviewed head pass. The Linux and macOS build workflows execute that exact command in their gating lint jobs, so as soon as the currently action-required workflows are approved every downstream build and test will be blocked. Please run `cargo fmt` on this call.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "unikdahal (via GitHub)" <gi...@apache.org>.
unikdahal closed pull request #5414: fix: support empty struct types
URL: https://github.com/apache/datafusion-comet/pull/5414
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3836460165
##########
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] 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 compactio
n is fixed.
##########
native/spark-expr/src/json_funcs/from_json.rs:
##########
@@ -150,27 +150,35 @@ fn json_string_to_struct(arr: &Arc<dyn Array>, schema: &DataType) -> Result<Arra
} else {
let json_str = string_array.value(row_idx);
- // Parse JSON (PERMISSIVE mode: return null fields on error)
- match serde_json::from_str::<serde_json::Value>(json_str) {
- Ok(json_value) => {
- if let serde_json::Value::Object(obj) = json_value {
- // Struct is not null, extract each field
- *struct_null = true;
- 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() {
Review Comment:
[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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3837170725
##########
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] 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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support empty struct types [datafusion-comet]
Posted by "unikdahal (via GitHub)" <gi...@apache.org>.
unikdahal commented on code in PR #5414:
URL: https://github.com/apache/datafusion-comet/pull/5414#discussion_r3838162016
##########
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:
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
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org