andygrove commented on code in PR #5750:
URL: https://github.com/apache/datafusion-comet/pull/5750#discussion_r4156343599
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -494,7 +496,46 @@ object CometSlice extends CometExpressionSerde[Slice] {
}
}
+private[comet] object ArraySetSupport {
+ val floatingPointReason: String =
+ "Floating-point array elements require Spark with SPARK-54918 " +
+ "(4.0.5+, 4.1.4+, or 4.2+) for matching signed-zero and NaN semantics"
+
+ // A top-level KnownFloatingPointNormalized marker is insufficient: Spark
also normalizes
+ // CreateArray, If, CaseWhen, and Coalesce recursively without wrapping the
resulting array.
+ def normalizesSignedZero(version: String): Boolean = {
Review Comment:
I think `normalizesSignedZero` returns `true` for releases where native
`array_distinct` and `array_union` still differ from Spark.
The gate assumes SPARK-54918, where `NormalizeFloatingNumbers` rewrites the
array argument in the plan. That is what `v4.2.0` has. Since then SPARK-59602
replaced that rewrite on every target branch (`branch-4.0` `7fff7b8889`,
`branch-4.1` `024f925076`, `branch-4.2` `de0f99ce2f`, and `master`). It removed
the `ArrayDistinct` and `ArrayUnion` cases from `NormalizeFloatingNumbers` and
moved the normalization into the expression itself. I checked the `v4.0.5-rc1`,
`v4.1.4-rc1`, `v4.1.4-rc2` and `v4.2.1-rc1` tags. None of them has the plan
rewrite. They all normalize inside `ArraySetLike` (`normalizedElement` and
`SQLOpenHashSet.withNaNCheckFunc`).
So on 4.0.5+, 4.1.4+ and 4.2.1+ Comet receives a bare `array_distinct(col)`
with no `KnownFloatingPointNormalized` wrapper and no `NormalizeNaNAndZero`.
Spark then normalizes while evaluating. The native path does not. DataFusion's
`general_array_distinct` only calls `normalize_float_zero` on a top-level float
values array, and `arrow-row` encodes `f64` from the raw bits, so NaNs with
different sign or payload stay distinct. I think these two cases diverge on
those releases:
- `size(array_distinct(array(d, -d)))` for a NaN column gives 1 in Spark and
2 in Comet. This is the same query as the new `array set noncanonical NaN
normalization` test.
- `array_distinct(array(array(0.0), array(-0.0)))`, or the same over
`struct<double>`. Spark dedups them and native does not.
These would now be reported as `Compatible()`, so there is no fallback and
no opt-in. CI only runs 4.0.4, 4.1.3 and 4.2.0, so it cannot see this. I expect
the NaN test and the `array_set_signed_zero.sql` fixture to fail on 4.2.1 as
soon as the Spark version is bumped.
Could we limit the native-by-default path to a Spark that still has the plan
rewrite (4.2.0 only), and keep `Incompatible` everywhere else? Alternatively
the native path could normalize NaN and nested zeros itself, for example by
reusing `NormalizeNestedFloats` and `normalize_float` on the input. That would
make `Compatible` true on the expression-level releases too. Either way I think
the version table in `floating-point.md` and `expressions.md`, the
`ArraySetSupport` constants, and the `4_0_4_1` fixture (whose
`spark_answer_only` cases would now be hiding the divergence) need to follow. A
test with a negated NaN column and a nested zero case that runs against 4.0.5,
4.1.4 or 4.2.1 would be the proof.
--
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]
