sunchao commented on code in PR #5452:
URL: https://github.com/apache/datafusion-comet/pull/5452#discussion_r3846862434
##########
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] 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:
```sql
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.
--
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]
