comphead commented on code in PR #6413:
URL: https://github.com/apache/datafusion-comet/pull/6413#discussion_r4146861499
##########
native/spark-expr/src/hash_funcs/xxhash64.rs:
##########
@@ -48,6 +48,8 @@ const SPARK_DEFAULT_SEED: i64 = 42;
/// mask into children, so hidden values of a NULL struct would affect the
hash
/// - a `Dictionary` nested in a list/map: `SparkXxhash64` restarts those
hashes from 42
/// - `Time64`, which `SparkXxhash64` does not dispatch
+/// - `Float32`/`Float64` (and anything containing one): `SparkXxhash64`
hashes the raw bits of a
Review Comment:
Would it make sense to also update the `xxhash64` entry in
`docs/source/contributor-guide/expression-audits/hash_funcs.md`? It lists where
the Comet kernel is kept (non-default seed, `Struct`, nested `Dictionary`,
`Time64`) and says the differential tests compare primitives with
`SparkXxhash64`. Floats are now another reason to keep the kernel, since their
NaN hashes differ.
##########
spark/src/test/resources/sql-tests/expressions/hash/hash.sql:
##########
@@ -30,3 +30,18 @@ SELECT md5(col), md5(cast(a as string)), md5(cast(b as
string)), hash(col), hash
-- native engine as scalar values rather than being folded away by Spark's
optimizer.
query
SELECT md5('Spark SQL'), sha1('test'), sha2('test', 0), sha2('test', 256),
sha2('test', 224), sha2('test', 384), sha2('test', 512), sha2('test', 128),
sha2('test', -1), sha2(cast(null as string), 256), hash('test'),
xxhash64('test')
+
+-- Spark hashes a float through doubleToLongBits or floatToIntBits, which
canonicalize NaN, so
+-- every NaN hashes alike. Negating a column flips the sign bit of a NaN,
giving the bits that
+-- arithmetic produces on x86-64.
+statement
+CREATE TABLE test_nan(d double, f float) USING parquet
+
+statement
+INSERT INTO test_nan VALUES (double('NaN'), float('NaN')), (0.0, 0.0),
(double('-0.0'), float('-0.0')), (1.5, 1.5), (NULL, NULL)
Review Comment:
Would it be worth adding `double('Infinity')` and `double('-Infinity')`
rows, plus the `float` equivalents? Then `hash(-d)` and `hash(-f)` would also
show that both infinities pass through the NaN canonicalization unchanged.
--
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]
