andygrove commented on code in PR #6989:
URL: https://github.com/apache/datafusion-comet/pull/6989#discussion_r4298435229
##########
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:
Fixed in 0870b3d8b4. Both messages now use `self.name()`, and a new test
checks that each variant's unsupported-type error names itself on the array and
scalar paths. Hardcoding either message makes it fail.
##########
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:
Agreed. The module doc now describes only the two equality policies and
points at `CometArraysOverlap.nativeFunction` for which Spark versions use
which, so the version knowledge stays in the serde. Fixed in 0870b3d8b4.
##########
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:
Reworded in 0870b3d8b4 to your suggestion: "so `-0.0` and `0.0` are one
value. Every NaN is one value in both variants."
##########
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:
Trimmed in 0870b3d8b4 to one version on each side of the boundary, 4.1.3 and
4.1.4, since `array set signed-zero Spark patch versions` already pins the
predicate. The `convert` assertion stays. I checked it still catches a
hardcoded name: with `convert` fixed to `spark_arrays_overlap_normalized` it
fails on 4.1.
--
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]
