sunchao commented on code in PR #6563:
URL: https://github.com/apache/datafusion-comet/pull/6563#discussion_r4186451005
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -540,35 +540,57 @@ object CometSlice extends CometExpressionSerde[Slice] {
private[comet] object ArraySetSupport {
val floatingPointReason: String =
- "Floating-point elements match Spark's signed-zero and NaN semantics
natively only on " +
- "Spark 4.2.0, whose optimizer normalizes the arguments (SPARK-54918)"
-
- // The native kernels match Spark only when the plan has already normalized
the arguments, and
- // only Spark 4.2.0 does that (SPARK-54918). Earlier releases keep flat
signed zeros apart.
- // From 4.0.5, 4.1.4 and 4.2.1, SPARK-59602 normalizes during evaluation
instead, which the
- // native kernels do not match for NaN payloads or nested zeros. A top-level
- // KnownFloatingPointNormalized marker cannot replace the version check:
Spark also normalizes
- // CreateArray, If, CaseWhen, and Coalesce recursively without wrapping the
resulting array.
- def normalizesArgumentsInPlan(version: String): Boolean =
- Utils.majorMinorPatchVersion(version).contains((4, 2, 0))
+ "Floating-point elements match Spark's signed-zero semantics natively only
on Spark " +
+ "4.0.5+, 4.1.4+ and 4.2+, which treat -0.0 and 0.0 as one value in these
functions " +
+ "(SPARK-54918, SPARK-59602)"
+
+ // Spark 4.2.0 normalizes the arguments of these functions in the plan
(SPARK-54918), and 4.0.5,
+ // 4.1.4 and 4.2.1 normalize while evaluating them (SPARK-59602). Either
way, Spark treats -0.0
+ // and 0.0, and every NaN, as one value at any depth, which the spark_
variants match. Earlier
+ // releases keep -0.0 and 0.0 apart in a flat array. The check reads the
version rather than a
+ // KnownFloatingPointNormalized marker, because SPARK-59602 adds no marker,
and SPARK-54918
+ // normalizes CreateArray, If, CaseWhen and Coalesce without wrapping the
resulting array.
+ def normalizesFloats(version: String): Boolean =
+ Utils.majorMinorPatchVersion(version).exists {
+ case (4, 0, patch) => patch >= 5
+ case (4, 1, patch) => patch >= 4
+ case (major, minor, _) => major > 4 || (major == 4 && minor >= 2)
+ }
def supportLevel(dataType: DataType): SupportLevel = {
- if (SupportLevel.containsType(dataType, classOf[FloatType],
classOf[DoubleType]) &&
- !normalizesArgumentsInPlan(SPARK_VERSION)) {
+ if (hasFloats(dataType) && !normalizesFloats(SPARK_VERSION)) {
Review Comment:
[P2] Preserve fallback for non-default collations when widening this gate.
On newly admitted Spark versions such as 4.1.4, an array of structs containing
both a double and a `STRING COLLATE UTF8_LCASE` now reports `Compatible()`.
Spark deduplicates structs containing `a` and `A` when their other fields
match, but `normalize_nested_floats` leaves strings unchanged and DataFusion
compares their bytes, so both new UDFs return two elements instead of one. The
previous float-type fallback protected these inputs. Check
`hasNonDefaultStringCollation(dataType)` recursively before enabling native
execution and add regression coverage for both functions.
Evidence: With a nullable Parquet DOUBLE column `d` containing `1.0`, use
`SELECT size(array_distinct(array(named_struct('s', IF(d IS NULL, NULL,
collate('a', 'UTF8_LCASE')), 'd', d), named_struct('s', IF(d IS NULL, NULL,
collate('A', 'UTF8_LCASE')), 'd', d)))) FROM t`. The verified Spark 4.1.4-rc2
source probe resolved this expression and returned `1`. The equivalent
`array_union` of the two singleton arrays also returned `1`. A disposable test
calling the exact-head `SparkArraySetOp` implementations with the corresponding
Arrow structs returned `2` for both functions. The analyzed struct fields are
nullable, so `CometCreateArray` does not insert a potentially rejecting cast.
`CometLiteral`, `CometIf`, and `CometCreateNamedStruct` admit the children,
while serialization drops collation metadata. This is a split Spark/native
reproduction, not a full Comet integration run.
--
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]
