Skip to content

feat: support Map for CreateArray literal - #5452

Open
comphead wants to merge 6 commits into
apache:mainfrom
comphead:create_array
Open

feat: support Map for CreateArray literal#5452
comphead wants to merge 6 commits into
apache:mainfrom
comphead:create_array

Conversation

@comphead

@comphead comphead commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #5448 .

Extends CometLiteral to serialize complex Literal values that Spark's ConstantFolding produces from map(...), named_struct(...), or array(...) sub-trees. The native Literal proto encodes scalars and nested ListLiteral only, so folded values containing MapType or StructType previously fell back to Spark.

Changes

  • CometLiteral.getSupportLevel reports Compatible for folded complex literals gated by canExpandComplexLiteral.
  • CometLiteral.convert calls expandComplexLiteral(value, dataType) which rebuilds the value as a tree of CreateArray / CreateMap / CreateNamedStruct over primitive-typed Literals, then recurses via exprToProtoInternal. Existing CometCreateArray / CometCreateMap / CometCreateNamedStruct serdes take over from there.
  • Non-null child literals are wrapped in KnownNullable. DataFusion make_array asserts strict Arrow-type equality across siblings and would panic on a nullability mismatch. CometKnownNullable is a no-op on the wire.
  • Empty top-level ArrayType literals stay on the pre-existing makeListLiteral path (element type cannot be recovered from a childless Create*).

@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

Reviewed ce1cfe973ee916f6e8de6373b7b4a48f99bcd776 against e0ab0a6fe60c05bd679654f0201ebe88319cc3a7. Reconstructing folded complex literals addresses the fallback reported in #5448, and the ordinary array-of-maps-of-arrays case executes successfully in a Comet projection. Three reproducible P2 correctness regressions remain: struct batch cardinality, struct-field nullability, and duplicate map-key handling.

Prior state and problem

Spark constant folding can replace an entire array(map(...), ...) expression with a complex Literal. The existing literal protocol encodes scalars and nested lists, but not populated maps or structs, so these folded values generally caused projection fallback even when the corresponding constructors were supported. The missing representation is therefore at the literal-serialization boundary, not simply in CreateArray argument dispatch.

Design approach

The change recognizes non-null literal types containing maps or structs and reconstructs CreateArray, CreateMap, or CreateNamedStruct expressions around their stored values. It then reuses ordinary expression serialization recursively. This avoids extending the literal protobuf; maps use the existing JVM codegen dispatcher inside the Comet pipeline, while arrays and structs use their native expression paths.

Correctness / compatibility analysis

The common map-of-array case matches Spark on both Spark 3.5.9 and 4.1.3. However, reconstruction must preserve the value's already-established semantics as well as its apparent Catalyst type. The inline findings show three places where that contract changes: scalar struct values become one-row arrays, field nullability is inferred again from individual values, and existing map entries are subjected to constructor duplicate-key validation.

Validation used make core before JVM tests, clean reactor builds when switching Spark profiles, and normal-folding probes over a three-row Parquet batch with native-scan/projection assertions. All seven probes pass with the exact base literal serializer substituted into the otherwise unchanged production build; at this head, five probes fail across the three reported causes and two controls pass, on both Spark versions. These were serializer differential controls, not separate full-base builds.

Key design decisions

Excluding empty top-level arrays and maps avoids synthesizing an untyped zero-argument constructor. Keeping primitive leaves on their existing serialization paths also limits the protocol surface. Both are useful constraints, but the KnownNullable wrapper is only a Catalyst annotation: its serializer forwards the child without carrying that annotation to the native struct expression. Likewise, reusing CreateMap is not equivalent to transporting arbitrary existing MapData.

Implementation sketch

needsExpansion identifies maps, structs, and arrays containing them; canExpandComplexLiteral gates admission; and convert delegates the reconstructed expression back to exprToProtoInternal. Array elements, map entries, and struct fields are read from Catalyst's internal containers. Nested complex children are expanded as they pass through the existing serdes, while unsupported children can still decline and retain Spark fallback.

Behavioral changes worth calling out

This enables standalone folded maps and structs, not only maps used as CreateArray children, so their existing native consumers become part of the compatibility boundary. The changed SQL fixture exercises constructor paths with ConstantFolding disabled by CometSqlFileTestSuite; it therefore does not validate the new folded-literal path. Separately, the fixture currently fails both dictionary variants on Spark 3.5.9 and 4.1.3 at line 67, where the non-folded map children have different value nullability. That failure exposes an existing constructor-path issue rather than proving that literal expansion is covered.

Suggested improvements

Preserve scalar/batch shape and struct-field nullability before admitting folded structs, and preserve existing map entries without reapplying constructor deduplication. Add normal-folding regression coverage over multirow inputs for the three inline cases, retaining assertions that the intended Comet path actually executes. Also resolve the newly failing fixture case and retain explicit empty/null-container fallback checks.

else asNullable(Literal(row.get(i, f.dataType), f.dataType))
Seq(Literal(f.name), v)
}
CreateNamedStruct(children.toSeq)

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 scalar/batch cardinality for reconstructed structs

With normal constant folding, SELECT id, named_struct('a', 1) FROM t now reaches native CreateNamedStruct instead of retaining Spark projection fallback. For all-scalar children, its evaluator calls ColumnarValue::values_to_arrays and returns a one-row StructArray, regardless of the input batch size. With three rows in one Parquet batch, the query fails with Array length 1 does not match expected length 3; arrays containing such structs similarly fail the make_array row-count check. I reproduced this on Spark 3.5.9 and 4.1.3, while the exact base literal serializer returns all three rows correctly. Please preserve scalar semantics/broadcast to the input batch size, or retain fallback for these literals until the native constructor supports them.

val children = fields.zipWithIndex.flatMap { case (f, i) =>
val v =
if (row.isNullAt(i)) Literal(null, f.dataType)
else asNullable(Literal(row.get(i, f.dataType), 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] Preserve struct-field nullability in the native representation

KnownNullable is discarded by CometKnownNullable.convert, and native CreateNamedStruct::fields infers each field's nullability from its serialized child. With normal folding, array(named_struct('a', CAST(NULL AS INT)), named_struct('a', 1)) is a single literal with a unified nullable struct type, so the outer CometCreateArray type guard passes. Recursive expansion then produces native struct fields with nullable=true and nullable=false. DataFusion 54.1 preserves those per-input flags during struct coercion, and Arrow panics in MutableArrayData with Arrays with inconsistent types. This reproduces on Spark 3.5.9 and 4.1.3 and the base serializer safely falls back. Please carry/normalize the field nullability on the native wire, or decline expansion when it cannot be preserved; the Catalyst-only wrapper does not prevent this panic.

else asNullable(Literal(vals.get(i, vt), vt))
Seq(k, v)
}
CreateMap(children, useStringTypeWhenEmpty = false)

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 existing map entries without reapplying constructor deduplication

A folded MapData need not have been produced by CreateMap. For example, Spark folds from_json('{"a":1,"a":2}', 'MAP<STRING,INT>') into a map literal containing keys [a,a] and values [1,2], and Spark/the base serializer execute it successfully. Rebuilding that value as CreateMap sends it through ArrayBasedMapBuilder again, so projecting it alongside a column from a Parquet table now throws [DUPLICATED_MAP_KEY] under the default policy. I reproduced the regression on Spark 3.5.9 and 4.1.3 with normal folding and a native Comet projection. Please preserve the literal's existing entries directly, or retain fallback when reconstruction would change their semantics.

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] Duplicate binary keys still evade the duplicate-key check

Retested 31f28b2e: the string-key example now falls back correctly, but casting those keys to binary still admits the literal:

SELECT id, CAST(
  from_json('{"a":1,"a":2}', 'MAP<STRING,INT>')
  AS MAP<BINARY,INT>) AS m
FROM t

Spark's map cast preserves both entries. The new .distinct check compares the extracted Array[Byte] keys by identity, whereas ArrayBasedMapBuilder compares binary keys by their contents using interpreted ordering. Reconstructing the admitted literal therefore still throws DUPLICATED_MAP_KEY under EXCEPTION. Under LAST_WIN, it silently drops the first entry instead of preserving the original map.

I reproduced both policies at this head on Spark 3.5.9 and 4.1.3 with normal folding, three-row Parquet input, and native CometProject assertions. Replacing only the literal serializer with the exact base version preserves both entries under either policy.

Could the duplicate check use Spark-compatible key equality and include this binary-key case under both deduplication policies?

Comment on lines +267 to +269
case MapType(kt, _, _) =>
val mapData = expr.value.asInstanceOf[MapData]
mapData.numElements() > 0 && !hasDuplicateMapKeys(mapData.keyArray(), kt)

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 map-key equality for newly admitted literals

A folded map can now reach native map_extract, whose Arrow-key comparison does not match Spark's key equality. With normal folding and Parquet id values 1, 2, 3, for example:

SELECT id, element_at(
  map(CAST(0 AS DOUBLE), 7),
  CAST(concat('-', CAST(id - 1 AS STRING), '.0') AS DOUBLE))
FROM t

On Spark 3.5.9, Spark returns 7, NULL, NULL, but this head returns NULL, NULL, NULL: the first lookup is -0.0, which must match the +0.0 key. The same regression occurs for array-of-double keys. Collated string keys hit this boundary too: on Spark 4.0.4 and 4.1.3, a folded MAP<STRING COLLATE UTF8_LCASE, INT> containing A1 -> 7 returns NULL for a dynamic a1 lookup instead of 7, because the wire type loses collation and native lookup is bytewise. These are single-key maps, not duplicate-key revalidation. All probes select CometProject at this head and return the correct results with the exact-base literal serializer retaining Spark projection. Please decline expansion for unsupported key semantics, or keep these lookups on a Spark-compatible path.

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.

checking, if so, means the current Spark test CI is not enough.

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] Nested map values still bypass the key-semantics guard

Retested 31f28b2e: the direct floating/collated-key cases now fall back correctly, but maps nested inside map values still escape the guard. With normal folding, ANSI mode enabled, and Parquet id values 1, 2, 3:

SELECT id, element_at(
  element_at(map(1, map(CAST(0 AS DOUBLE), 7)), id),
  CAST(concat('-', CAST(id - 1 AS STRING), '.0') AS DOUBLE)) AS v
FROM t

Spark returns 7, NULL, NULL, while the native Comet projection returns NULL, NULL, NULL on both Spark 3.5.9 and 4.1.3. The outer key type is INT, so it passes the guard. CometCreateMap then closure-serializes the whole expression through the JVM dispatcher. The inner map literal never revisits CometLiteral, leaving its double keys available to native map_extract. A positive-zero lookup control passes, and the native lookup-key expression was checked to produce negative zero for the first row.

Nested UTF8_LCASE map keys have the same bypass on Spark 4.1.3: an A1 -> 7 entry fails a dynamic a1 lookup, while the exact-case control passes. Replacing only the literal serializer with the exact base version restores the expected results.

Could the admission check inspect every contained map's key type, including maps inside values, or keep their lookups on a Spark-compatible path? Please cover a dynamic outer lookup so constant folding cannot extract the inner map into a separately guarded literal.

@comphead

Copy link
Copy Markdown
Contributor Author

Thanks @sunchao addressed comments

} else {
val values = mapData.valueArray()
val children = (0 until keys.numElements()).flatMap(i =>
Seq(literalAt(keys, i, kt), asNullable(literalAt(values, i, vt))))

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 map-value nullability when expanding folded literals

Could reconstruction retain the literal's declared valueContainsNull? With normal constant folding and a Parquet table containing id values 1, 2, 3:

SELECT id, array(map(1, 2), map(2, coalesce(id, 0))) AS a
FROM t

Both original array children have MapType(IntegerType, IntegerType, false), so the existing array type check passes. Wrapping the folded map's value in KnownNullable changes only that reconstructed map to valueContainsNull=true. The dynamic sibling stays false, and native make_array panics with Arrays with inconsistent types passed to MutableArrayData. Unlike the earlier non-folded constructor example, this rewrite introduces the mismatch after checking two identical input types.

I reproduced this at 31f28b2e on Spark 3.5.9 and 4.1.3 with native CometProject assertions. Replacing only the literal serializer with the exact base version restores Spark projection and the correct three-row result. Please preserve the declared map-value nullability and add a normal-folding regression test for this mixed literal/dynamic case.

* `UTF8_BINARY`. Both are checked at every nesting level of the key type.
*/
private def hasUnsafeMapKeyType(kt: DataType): Boolean =
hasNonDefaultStringCollation(kt) ||

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 nullable components of complex map lookup keys

This admits folded maps whose key type is ArrayType(IntegerType, false), but Spark permits a dynamic lookup key containing a null element. With normal folding and Parquet id values 1, 2, 3:

SELECT id, element_at(
  map(array(1), 7),
  array(IF(id = 2, CAST(NULL AS INT), id))) AS v
FROM t

Spark returns 7, NULL, NULL. Native map_extract instead coerces the lookup to the literal key's exact Arrow type; the inserted cast aborts with Non-nullable field of ListArray "item" cannot contain nulls. The null is inside the lookup array, not a null map key. This needs a nullability-compatible lookup type or fallback for these literals.

Reproduced on Spark 3.5.9 and 4.1.3 with native CometProject assertions, in both ANSI modes. The non-null lookup control and dispatcher-off path pass; replacing only the literal serializer with the exact base version also restores the correct rows. No floating-point keys, collations, or duplicate keys are involved.

None
} else {
val values = mapData.valueArray()
val children = (0 until keys.numElements()).flatMap(i =>

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 map literals before native map_entries

A map stored inside a map value is handed to the JVM dispatcher as a literal, so its original valueContainsNull=false survives. This enables another previously falling-back query with normal folding over Parquet id values 1, 2, 3:

SELECT id, map_entries(
  element_at(map(1, map(1, 2)), id)) AS e
FROM t

The inner map reaches native map_entries, which declares a nullable value field but reuses entry arrays whose field is non-nullable. Arrow's ListArray::new then panics with a child-type mismatch instead of returning Spark's [{1, 2}], NULL, NULL. There are no mixed map siblings here: the inner type has not been changed; the incompatible schema is created by its consumer.

Reproduced on Spark 3.5.9 and 4.1.3 with native CometProject assertions, in both ANSI modes. Nullable-inner-map, map_keys, and map_values controls pass; the exact-base literal serializer and dispatcher-off path restore Spark's result. Please retain fallback for unsupported nested-map shapes until their native consumers preserve matching entry types.

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

Re-reviewed 97544ebe6e60ef558ebb5dc0f6b34aa7ec1ea242 against e0ab0a6fe60c05bd679654f0201ebe88319cc3a7, including the increment from 31f28b2e0ea5341716f2824be3b7a26bc8c82acb and all prior discussions. Three additional P2 regressions are attached below.

The existing [P2] key-equality family (original thread, nested-map follow-up) also remains reproducible through map_contains_key, although the reported element_at/subscript cases are now guarded. With Parquet id values 1, 2, 3 and normal folding:

SELECT id, map_contains_key(
  element_at(map(1, map(CAST(0 AS DOUBLE), 7)), id),
  CAST(concat('-', CAST(id - 1 AS STRING), '.0') AS DOUBLE)) AS present
FROM t

Spark returns true, NULL, NULL; Comet returns false, NULL, NULL on Spark 3.5.9 and 4.1.3. Spark lowers membership to array_contains(map_keys(...), key), bypassing MapKeySupport. The analogous nested UTF8_LCASE map with stored A1 and dynamic a1 lookup also returns false on 4.1.3. Positive-zero and exact-case controls pass. I am keeping this with the existing family rather than adding another inline finding.

Validation used the exact head/base Scala serde sources compiled into isolated overlays over existing native builds, with normal folding, multirow Parquet input, and native scan/projection assertions. The relevant native paths were unchanged; this was not a full current-head native rebuild, and I did not locally rerun the changed map_entries planner. Base controls passed all 7 applicable probes on 3.5.9 and all 9 on 4.1.3; current-source probes reproduced the findings above and below. Current GitHub checks are green.

Comment on lines +328 to +330
val ordering = TypeUtils.getInterpretedOrdering(keyType)
val sorted = (0 until n).map(i => keys.get(i, keyType)).sorted(ordering)
sorted.sliding(2).exists(pair => ordering.compare(pair.head, pair.last) == 0)

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] Decline non-orderable map keys before requesting an ordering

TypeUtils.getInterpretedOrdering is not defined for every valid Spark map-key type. Spark supports CalendarIntervalType keys using hash equality, so normal folding produces a valid two-entry literal for:

SELECT id, map(make_interval(1), 1, make_interval(2), 2) AS m FROM t

With Parquet id values 1, 2, 3, this new call throws Type PhysicalCalendarIntervalType does not support ordered operations from CometLiteral.getSupportLevel, before the dispatcher can decline the unsupported type. The keys are distinct and non-null. I reproduced the planning failure with the exact current serde on Spark 3.5.9 and 4.1.3; Spark and the exact-base serde return all three rows correctly. Please retain fallback for non-orderable key types, or use the matching Spark hash/equality path instead of unconditionally requesting an ordering.

Comment on lines +520 to +524
case MapType(keyType, valueType, valueContainsNull) =>
MapType(
normalizeContainerNullability(keyType),
normalizeContainerNullability(valueType),
valueContainsNull = true)
valueContainsNull)

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 array nullability inside map types significant

The recursion still erases ArrayType.containsNull inside a MapType, even though DataFusion 54.1 cannot coerce two differing map types. With normal folding and a Parquet id INT column containing 1, 2, 3:

SELECT id, array(map(1, array(1)), map(2, array(id))) AS a FROM t

The folded map has value type ArrayType(IntegerType,false) and the dynamic sibling has ArrayType(IntegerType,true). Both maps retain valueContainsNull=false. Normalization admits them, but native make_array receives unequal Arrow map types and panics with Arrays with inconsistent types passed to MutableArrayData. This reproduces with the exact current serde on Spark 3.5.9 and 4.1.3 with CometProject asserted; replacing id with coalesce(id,0) passes, as does the exact-base serde retaining Spark projection. Unlike the earlier valueContainsNull finding, reconstruction preserves both original map flags here. Please preserve nested array nullability across the map boundary, or unify the complete map types before calling make_array.

Comment on lines +276 to +280
val children = (0 until keys.numElements()).flatMap(i =>
Seq(
literalAt(keys, i, kt),
withNullability(literalAt(values, i, vt), valueContainsNull)))
Some(CreateMap(children, useStringTypeWhenEmpty = false))

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 NULL short-circuiting for newly admitted map lookups

Admitting the nested map literal exposes eager evaluation of a lookup key that Spark skips after a NULL map. With ANSI enabled and Parquet id values 1, 2, 3:

SELECT id, element_at(
  element_at(map(1, map(0, 7)), id),
  id % (id - 2)) AS v
FROM t

Spark returns 7, NULL, NULL: for id=2, the inner lookup returns NULL, so the outer ElementAt never evaluates the remainder. The native map_extract scalar expression evaluates every argument over the batch first and raises a divide/remainder-by-zero error. All map keys are integers, so the new key-type guards do not prevent it. I reproduced this with the exact current serde on Spark 3.5.9 and 4.1.3 with CometProject asserted; the exact-base serde and a nonzero-divisor control pass. Please preserve per-row lazy key evaluation, or retain fallback for these newly admitted expressions until the native consumer has Spark's NULL behavior.

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

Re-reviewed fecf175ab8f7ad434fa53a255851d4df90349728 against e0ab0a6fe60c05bd679654f0201ebe88319cc3a7, including the increment from 97544ebe6e60ef558ebb5dc0f6b34aa7ec1ea242 and all prior discussions. One additional [P2] is attached: the recursive map_entries cast changes nested map types and can cause a downstream make_array panic. This is in the full PR diff, not introduced by the latest refactor.

The existing [P2] findings still reproduce: interval-key ordering, array nullability inside map values, NULL-map eager key evaluation, and the key-equality family / nested-map follow-up through map_contains_key, as documented in review 5023100588. No duplicate inline findings for these.

Fresh exact-serde probes on Spark 3.5.9 and 4.1.3 reproduced those existing failures; all base and positive controls passed. The new finding has a passing native SQL control with the pre-change planner plus a differential DataFusion 54.1 / Arrow 58.4 kernel reproduction using the exact current cast helper and actual Comet extraction kernels. These are focused controls over existing native builds, not a full current-head native rebuild.

Comment thread native/core/src/execution/planner.rs Outdated
// `coerce_child_fields_nullable`.
let args = if fun_name == "map_entries" {
args.into_iter()
.map(|arg| Self::coerce_child_fields_nullable(arg, &input_schema))

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] Widen only the outer map_entries value field

This recursive cast also changes nullability inside the map's key/value types, although map_entries only needs the outer entry's value field to be nullable. With normal folding and Parquet id INT values 1, 2, 3:

SELECT id, array(
  map_entries(map(1, IF(id = 1, map(1, 2), NULL)))[0].value,
  map(2, coalesce(id, 0))) AS a
FROM t

Both Catalyst array children are MapType(INT, INT, false), so the array guard accepts them. The outer map already has valueContainsNull=true, and the pre-change native query returns all three rows correctly. This cast unnecessarily widens the inner map to valueContainsNull=true; ListExtract and GetStructField preserve that changed type while the sibling remains false. DataFusion 54.1 cannot coerce those unequal map types, and make_array panics with Arrays with inconsistent types passed to MutableArrayData.

Please preserve the nested key/value types and widen only the outer entry-value flag. This is distinct from the original map_entries child-type failure: that call succeeds here, and the new cast introduces a mismatch in its downstream consumer. Verified with a passing native SQL control and a differential kernel probe using the exact cast helper, DataFusion's cast/map_entries/make_array, and Comet's extraction kernels; this was not a full current-head native rebuild.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support MapType(IntegerType,ArrayType(IntegerType,false),true) for CreateArray

2 participants