viirya commented on code in PR #6989:
URL: https://github.com/apache/datafusion-comet/pull/6989#discussion_r4286179742
##########
native/spark-expr/src/array_funcs/arrays_overlap.rs:
##########
@@ -68,13 +80,27 @@ impl SparkArraysOverlap {
pub fn new() -> Self {
Self {
signature: Signature::new(TypeSignature::Any(2),
Volatility::Immutable),
+ normalize_floats: false,
+ }
+ }
+
+ /// Normalizes flat `FLOAT` and `DOUBLE` elements before matching them, so
`-0.0` and `0.0` are
+ /// one value as well as every NaN.
+ pub fn normalized() -> Self {
+ Self {
+ normalize_floats: true,
+ ..Self::new()
}
}
}
impl ScalarUDFImpl for SparkArraysOverlap {
fn name(&self) -> &str {
- "spark_arrays_overlap"
+ if self.normalize_floats {
+ "spark_arrays_overlap_normalized"
Review Comment:
Nit: the `exec_err!` messages in `invoke_with_args` (lines 155 and 178)
still hardcode `spark_arrays_overlap does not support types ...`, and are now
also raised by `spark_arrays_overlap_normalized`. Using `self.name()` there
would keep them accurate.
##########
native/spark-expr/src/array_funcs/arrays_overlap.rs:
##########
@@ -68,13 +80,27 @@ impl SparkArraysOverlap {
pub fn new() -> Self {
Self {
signature: Signature::new(TypeSignature::Any(2),
Volatility::Immutable),
+ normalize_floats: false,
+ }
+ }
+
+ /// Normalizes flat `FLOAT` and `DOUBLE` elements before matching them, so
`-0.0` and `0.0` are
+ /// one value as well as every NaN.
Review Comment:
Nit: "so `-0.0` and `0.0` are one value as well as every NaN" can be read as
"`-0.0` and `0.0` are the same value as every NaN". Maybe: "so `-0.0` and `0.0`
are one value. Every NaN is one value in both variants."
##########
native/spark-expr/src/array_funcs/arrays_overlap.rs:
##########
@@ -28,6 +28,13 @@
//! - true if any non-null element appears in both arrays
//! - null if no definite overlap but either array contains null elements
//! - false if no overlap and neither array contains null elements
+//!
+//! Spark looks up flat elements in a hash set of boxed values, so every NaN
is one value, while
+//! `-0.0` and `0.0` are two. Spark 4.2.0 normalizes the arguments in the plan
(SPARK-54918), and
+//! 4.0.5, 4.1.4 and 4.2.1 normalize flat `FLOAT` and `DOUBLE` elements before
the lookup
+//! (SPARK-59602), which makes the zeros one value too.
`spark_arrays_overlap_normalized` matches
+//! them that way, and the serde picks it on those versions. Nested elements
compare in Spark's SQL
+//! ordering on every version, where the zeros are equal.
Review Comment:
This repeats the Spark version mapping that already lives in
`CometArraysOverlap.nativeFunction`, `ArraySetSupport`, `floating_point.md` and
the audit doc. Since the native side only implements the two equality policies
and the serde picks between them, could this paragraph describe just that and
point to the serde for which versions use which? That keeps the version
knowledge in one place when the next patch release changes it.
##########
spark/src/test/scala/org/apache/comet/CometArrayExpressionSuite.scala:
##########
@@ -771,6 +771,28 @@ class CometArrayExpressionSuite extends CometTestBase with
AdaptiveSparkPlanHelp
assert(nullableDelimiter.isDefined && !nullableDelimiter.get.hasIf)
}
+ test("arrays_overlap native variant by Spark version") {
+ // "array set signed-zero Spark patch versions" pins the version
boundaries. Where Spark
+ // normalizes, -0.0 and 0.0 are one value in a flat FLOAT or DOUBLE array,
which no pinned
+ // Spark profile can show: 4.2.0 normalizes the arguments in the plan,
before the native
+ // kernel sees them.
+ Seq("3.4.3", "3.5.9", "4.0.4", "4.1.3").foreach { version =>
+ assert(CometArraysOverlap.nativeFunction(version) ==
"spark_arrays_overlap", version)
+ }
+ Seq("4.0.5", "4.1.4", "4.2.0", "4.2.1").foreach { version =>
+ assert(
+ CometArraysOverlap.nativeFunction(version) ==
"spark_arrays_overlap_normalized",
+ version)
Review Comment:
The comment above says "array set signed-zero Spark patch versions" already
pins these boundaries, and `nativeFunction` is a single `if` over
`normalizesFloats`. So this eight-version table mostly re-tests that predicate.
The `convert` check below is the part that adds coverage, because it fails if
`convert` hardcodes a name. I'd keep one version on each side, or drop the
table and keep just the `convert` assertion.
--
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]
