sunchao commented on code in PR #5452:
URL: https://github.com/apache/datafusion-comet/pull/5452#discussion_r3848126069
##########
spark/src/main/scala/org/apache/comet/serde/literals.scala:
##########
@@ -215,4 +224,107 @@ object CometLiteral extends CometExpressionSerde[Literal]
with Logging {
}
listLiteralBuilder
}
+
+ /**
+ * True when a non-null Literal of this type is not encodable in the native
`Literal` proto and
+ * `convert` should try to rebuild it from primitive-typed Literals. The
native proto today
+ * carries scalars and nested `ListLiteral`s (arrays of arrays / arrays of
scalars). It does not
+ * carry Map values, so any Literal whose type contains a `MapType` needs to
be expanded before
+ * serialization.
+ *
+ * `StructType` (at any nesting depth) is deliberately excluded. Native
`CometCreateNamedStruct`
+ * (`spark/src/main/scala/org/apache/comet/serde/structs.scala`) uses
`values_to_arrays` in
+ * `native/spark-expr/src/struct_funcs/create_named_struct.rs`, which
returns a 1-row
+ * `StructArray` whenever all children are scalar values. That collides with
the row count of
+ * the surrounding batch and fails `make_array`'s length check. Field
nullability is likewise
+ * inferred from the concrete child expressions, and `CometKnownNullable`
+ *
(`spark/src/main/scala/org/apache/comet/serde/contraintExpressions.scala:99`)
drops the tag
+ * on the wire, so wrapping a non-null child in `KnownNullable` does not
carry across. Fall back
+ * to Spark for those shapes.
+ */
+ private def needsExpansion(dataType: DataType): Boolean = dataType match {
+ case _: MapType => true
+ case ArrayType(et, _) => needsExpansion(et)
+ case _ => false
+ }
+
+ /**
+ * True when the Literal is a non-null complex value that we can rebuild
from primitive Literals
+ * via [[expandComplexLiteral]]. Empty top-level containers are excluded
because a synthesized
+ * `Create[Array|Map]` with no children cannot recover the original element
type. A folded
+ * `MapData` with duplicate keys is also excluded: Spark's `CreateMap.eval`
+ * (`sql/catalyst/.../complexTypeCreator.scala:250`) feeds every entry
through
+ * `ArrayBasedMapBuilder`, which throws under the default
`MAP_KEY_DEDUP_POLICY=EXCEPTION`
+ * (`sql/catalyst/.../util/ArrayBasedMapBuilder.scala`). Rebuilding a folded
literal that came
+ * from `from_json` or a similar source would then throw where the original
literal had executed
+ * cleanly.
+ */
+ private def canExpandComplexLiteral(expr: Literal): Boolean = {
+ if (expr.value == null) return false
+ expr.dataType match {
+ case at: ArrayType if needsExpansion(at) =>
+ expr.value.asInstanceOf[ArrayData].numElements() > 0
+ case MapType(kt, _, _) =>
+ val mapData = expr.value.asInstanceOf[MapData]
+ mapData.numElements() > 0 && !hasDuplicateMapKeys(mapData.keyArray(),
kt)
Review Comment:
[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:
```sql
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.
##########
spark/src/main/scala/org/apache/comet/serde/literals.scala:
##########
@@ -215,4 +225,82 @@ object CometLiteral extends CometExpressionSerde[Literal]
with Logging {
}
listLiteralBuilder
}
+
+ /**
+ * True when a non-null Literal of this type is not encodable in the native
`Literal` proto and
+ * `convert` should try to rebuild it from primitive-typed Literals. The
native proto today
+ * carries scalars and nested `ListLiteral`s (arrays of arrays / arrays of
scalars). It does not
+ * carry Map or Struct values, so any Literal whose type contains a
`MapType` or a `StructType`
+ * needs to be expanded before serialization.
+ */
+ private def needsExpansion(dataType: DataType): Boolean = dataType match {
+ case _: MapType | _: StructType => true
+ case ArrayType(et, _) => needsExpansion(et)
+ case _ => false
+ }
+
+ /**
+ * True when the Literal is a non-null complex value that we can rebuild
from primitive Literals
+ * via [[expandComplexLiteral]]. Empty top-level containers are excluded
because a synthesized
+ * `Create[Array|Map]` with no children cannot recover the original element
type.
+ */
+ private def canExpandComplexLiteral(expr: Literal): Boolean = {
+ val value = expr.value
+ if (value == null || !needsExpansion(expr.dataType)) return false
+ expr.dataType match {
+ case _: ArrayType => value.asInstanceOf[ArrayData].numElements() > 0
+ case _: MapType => value.asInstanceOf[MapData].numElements() > 0
+ case _: StructType => true
+ case _ => false
+ }
+ }
+
+ /**
+ * Rebuild a folded complex Literal as an equivalent tree of `CreateArray` /
`CreateMap` /
+ * `CreateNamedStruct` over primitive-typed Literals. Callers must gate on
+ * [[canExpandComplexLiteral]] so `dataType` is one of the three complex
cases and top-level
+ * containers are non-empty.
+ */
+ private def expandComplexLiteral(value: Any, dataType: DataType): Expression
=
+ dataType match {
+ case ArrayType(et, _) =>
+ val arr = value.asInstanceOf[ArrayData]
+ val elems = (0 until arr.numElements()).map { i =>
+ if (arr.isNullAt(i)) Literal(null, et)
+ else asNullable(Literal(arr.get(i, et), et))
+ }
+ CreateArray(elems, useStringTypeWhenEmpty = false)
+ case MapType(kt, vt, _) =>
+ val mapData = value.asInstanceOf[MapData]
+ val keys = mapData.keyArray()
+ val vals = mapData.valueArray()
+ val children = (0 until keys.numElements()).flatMap { i =>
+ val k = if (keys.isNullAt(i)) Literal(null, kt) else
Literal(keys.get(i, kt), kt)
+ val v =
+ if (vals.isNullAt(i)) Literal(null, vt)
+ else asNullable(Literal(vals.get(i, vt), vt))
+ Seq(k, v)
+ }
+ CreateMap(children, useStringTypeWhenEmpty = false)
Review Comment:
[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:
```sql
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
