viirya commented on code in PR #6563:
URL: https://github.com/apache/datafusion-comet/pull/6563#discussion_r4213052063
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -540,39 +540,73 @@ 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)"
+
+ val collationReason: String =
+ "Elements that hold both a floating-point value and a non-UTF8_BINARY
collated string fall " +
+ "back to Spark, which compares the strings under their collation, while
Comet's native " +
+ "kernels compare their raw bytes"
+
+ // 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
Review Comment:
I agree that a float-only fallback would be the wrong fix here. Reverting
the guard was the right call given the subtree duplication, and #6716
evaluating the left operand once is the shape this needs.
My concern is merge order. Against `branch-1.1`, a user on 4.0.5, 4.1.4 or
4.2.1 gets Spark's NULL for `array_union(a, slice(b, 0, 1))` with
`ARRAY<DOUBLE>` when `a` is NULL, because the float fallback keeps it on Spark.
With this PR alone, they get the `slice` error instead. #6716 is still open and
currently has conflicts with `main`. Could we land #6716 first and merge this
one after it? Then no Comet release ships the wider native path without the
short-circuit. If you'd rather not wait, an alternative is to add a test in
#6716 that covers the `ARRAY<DOUBLE>` case on a 4.1.4+ version string, so the
dependency is explicit.
##########
spark/src/test/resources/sql-tests/expressions/array/array_set_signed_zero_spark_4_0_4_1.sql:
##########
@@ -18,9 +18,10 @@
-- MinSparkVersion: 4.0
-- MaxSparkVersion: 4.1
--- 4.0.0-4.0.4 and 4.1.0-4.1.3 keep signed zeros distinct. 4.0.5+ and 4.1.4+
normalize during
--- evaluation (SPARK-59602), which native distinct/union do not match for NaNs
or nested zeros, so
--- distinct/union fall back on every 4.0 and 4.1 release.
+-- 4.0.0-4.0.4 and 4.1.0-4.1.3 keep signed zeros distinct, so distinct/union
fall back there.
+-- 4.0.5+ and 4.1.4+ normalize during evaluation (SPARK-59602), and
distinct/union run natively.
+-- Which path runs depends on the patch release, so these cases check only the
answer;
Review Comment:
The switch to `spark_answer_only` makes sense, since the path here depends
on the patch release. It does mean that on the versions CI pins (4.0.4 and
4.1.3) these cases fall back and compare Spark with itself. On 4.2.0 the plan
normalizes the arguments before Comet sees them, so the native normalization
never changes the answer there. The opt-in arms in `CometArrayExpressionSuite`
exercise it, but their inputs are chosen so that older Spark agrees: positive
zero first, and `size` for the NaN cases.
So nothing in CI runs the default native path against a Spark that
normalizes during evaluation, and that is the case this PR enables. Could you
run the `expressions/array/` fixtures, `CometArrayExpressionSuite` and
`CometFloatSemanticsSuite` against `4.1.4-rc2` from the Apache staging
repository, and ideally `4.0.5-rc1` as well, and post the result here? On those
versions this fixture and the signed-zero cases in the suite take the native
path, so the run would check the claim directly.
--
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]
