andygrove commented on code in PR #6563:
URL: https://github.com/apache/datafusion-comet/pull/6563#discussion_r4219045380
##########
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:
Agreed, this one waits for #6716. When it's in, I'll merge main here and add
your query over `ARRAY<DOUBLE>` as a test that expects native execution
wherever float elements run natively. CI's 4.2.0 job then runs it through
`spark_array_union` with the short-circuit, which #6716's fixture doesn't
reach, since it only covers `ARRAY<INT>`.
##########
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:
I ran them on this branch with main merged (93b7cf45c) against the staged
artifacts for `v4.1.4-rc2` (`orgapachespark-1533`) and `v4.0.5-rc1`
(`orgapachespark-1534`), and also `v4.2.1-rc1` (`orgapachespark-1531`), the
first 4.2 release that no longer normalizes in the plan. Each staging repo's
`spark-version-info.properties` matches its tag's commit. The runs covered the
`expressions/array/` fixtures, `CometArrayExpressionSuite`,
`CometFloatSemanticsSuite` and `SqlFileTestParserSuite`.
Every `array_distinct` and `array_union` test passes on all three. On these
versions the signed-zero, NaN and nested tests in `CometArrayExpressionSuite`
take their `checkSparkAnswerAndOperator` branch, so they assert native
execution, and on 4.2.1 the 4.2+ fixture runs its default native path. For this
fixture I ran a copy with the `spark_answer_only` cases switched to `query`,
which asserts native execution, and it passes on 4.1.4-rc2 and 4.0.5-rc1.
The same four other tests fail on all three RCs. They fail the same way on
main at f80042f78 against 4.1.4-rc2, because other changes in these releases
cause them:
- `arrays_overlap.sql` and the `arrays_overlap [double]` and `[float]` cases
of `CometFloatSemanticsSuite`. SPARK-59602 also makes `arrays_overlap` treat
`-0.0` and `0.0` as one value, so `arrays_overlap(array(0.0D), array(-0.0D))`
is now `true` in Spark, while the native flat path still keeps the zeros apart
and returns `false`. Filed as #6768.
- `sequence.sql`. SPARK-58821 makes `sequence(-9223372036854775808L,
9223372036854775807L, 9223372036854775807L)` return the sequence instead of
raising `Unreachable code reached.`, which the native kernel still reproduces.
Filed as #6769.
| Run | Passed | Failed |
| --- | --- | --- |
| 4.1.4-rc2 | 576 | 4 |
| 4.0.5-rc1 | 576 | 4 |
| 4.2.1-rc1 | 576 | 4 |
| main, 4.1.4-rc2 | 573 | 4 |
The branch counts include the native copy of this fixture, and every count
includes the fixtures that skip themselves on that version. On the pinned 4.1.3
and 4.2.0, all 579 pass.
--
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]
