comphead commented on code in PR #5452:
URL: https://github.com/apache/datafusion-comet/pull/5452#discussion_r3847067767
##########
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:
checking, if so, means the current Spark test CI is not enough.
--
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]
