kosiew commented on code in PR #25971:
URL: https://github.com/apache/datafusion/pull/25971#discussion_r4335631996
##########
datafusion/spark/src/function/hash/xxhash64.rs:
##########
@@ -532,4 +532,34 @@ mod tests {
}
assert_ne!(hashes[0], hashes[1]);
}
+
+ /// Spark hashes floats through `floatToIntBits` / `doubleToLongBits`,
which
+ /// map every NaN to the canonical one, so a NaN with the sign bit set or
+ /// another payload hashes like `NAN`. Expected values are Spark's, from
+ /// apache/datafusion#25913.
+ #[test]
+ fn test_xxhash64_nan_f32() {
+ use arrow::array::Float32Array;
+ let array: ArrayRef = Arc::new(Float32Array::from(vec![
+ f32::NAN,
+ f32::from_bits(0xffc0_0000),
+ f32::from_bits(0x7f80_0001),
+ ]));
+ let mut hashes = vec![DEFAULT_SEED; 3];
+ create_xxhash64_hashes(&[array], &mut hashes).unwrap();
+ assert_eq!(hashes, vec![2692338816207849720u64; 3]);
+ }
+
+ #[test]
+ fn test_xxhash64_nan_f64() {
+ use arrow::array::Float64Array;
+ let array: ArrayRef = Arc::new(Float64Array::from(vec![
+ f64::NAN,
+ f64::from_bits(0xfff8_0000_0000_0000),
+ f64::from_bits(0x7ff0_0000_0000_0001),
+ ]));
+ let mut hashes = vec![DEFAULT_SEED; 3];
Review Comment:
Optional: Consider testing NaNs after another argument with different
preceding values per row. This would verify that canonicalization preserves
each row's accumulated hash rather than resetting to `DEFAULT_SEED`. The
current implementation appears correct; this is regression hardening and could
share a test with the nullable cases.
##########
datafusion/spark/src/function/hash/utils.rs:
##########
@@ -201,6 +202,12 @@ macro_rules! hash_array_primitive_float {
// Spark uses 0 as hash for -0.0, see `Murmur3Hash` expression.
if v == 0.0 && v.is_sign_negative() {
(0 as $ty2).to_le_bytes()
+ }
+ // Spark hashes `floatToIntBits` / `doubleToLongBits`, which
map every
+ // NaN to the canonical one (0x7fc00000 / 0x7ff8000000000000),
the same
+ // bits as Rust's `f32::NAN` / `f64::NAN`.
+ else if v.is_nan() {
+ <$ty>::NAN.to_le_bytes()
Review Comment:
The optimized Float32/Float64 list transforms at lines 342–350 and 437–445
still hash raw NaN bits, bypassing this fix. Please canonicalize NaNs in both
offset-based and fixed-size list paths while preserving negative-zero handling,
null skipping, and running seeds. Add tests for both widths across List,
LargeList, and FixedSizeList, including noncanonical NaNs, preferably through
`SparkXxhash64::invoke_with_args`. A singleton negative Float32 NaN list
currently hashes to `4760557555880201639` instead of Spark's
`2692338816207849720`.
##########
datafusion/spark/src/function/hash/xxhash64.rs:
##########
@@ -532,4 +532,34 @@ mod tests {
}
assert_ne!(hashes[0], hashes[1]);
}
+
+ /// Spark hashes floats through `floatToIntBits` / `doubleToLongBits`,
which
+ /// map every NaN to the canonical one, so a NaN with the sign bit set or
+ /// another payload hashes like `NAN`. Expected values are Spark's, from
+ /// apache/datafusion#25913.
+ #[test]
+ fn test_xxhash64_nan_f32() {
+ use arrow::array::Float32Array;
+ let array: ArrayRef = Arc::new(Float32Array::from(vec![
Review Comment:
Optional: Could we add nullable NaN cases for both float widths? The current
tests cover only the no-null fast path. Verify that canonical and noncanonical
NaNs produce the expected hash while null rows retain their incoming seed.
--
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]
