jinjin1919 opened a new pull request, #7569:
URL: https://github.com/apache/datafusion-comet/pull/7569
## Which issue does this PR close?
Part of #6984.
This covers only the Map-arm half of the issue. The string/binary list half
is being handled
separately by @med9110, so this PR should not close the issue on its own.
## Rationale for this change
In `create_hashes_internal!`, the Map arm hand-writes the same hashing loop
four times, once each
for (Utf8, Int32), (Int32, Utf8), (Utf8, Utf8) and (Int32, Int32). Each copy
has a loop for input
without nulls and a null-aware loop. They differ only in the downcast types
and in whether
`.to_le_bytes()` is applied to an `Int32` side. So any change to how map
entries are hashed has to
be made eight times, and hash results have to keep matching Spark exactly.
## What changes are included in this PR?
- A new `hash_map_entries!` macro in
`native/spark-expr/src/hash_funcs/utils.rs`, modelled on
`hash_list_primitive!`. It takes the offsets, the map, key and value
arrays, the hash buffer and
hash function, and one transform for the key and one for the value.
- The four specialised arms now each downcast their key and value arrays and
call the macro.
String sides pass `|v| v`, since `&str` is already `AsRef<[u8]>`. `Int32`
sides pass
`|v: i32| v.to_le_bytes()`.
- Behaviour is unchanged: the condition for taking the fast path (map, keys
and values all
null-free), hashing the key before the value in each entry, and skipping
null map rows, null
keys and null values each on its own.
- The fallback arm for other key/value types (`_ =>`) is not touched.
- A new test, `test_specialised_map_pairs_match_spark`, in
`hash_funcs/murmur3.rs` (see below).
## How are these changes tested?
**Existing tests.** `cargo test -p datafusion-comet-spark-expr hash_funcs`:
95 passed, 0 failed.
That is all 94 existing murmur3 and xxhash64 tests, including the four
`xxhash64_diff` map tests
that compare against upstream `SparkXxhash64`, plus the new test.
**New test.** The existing map tests cover only the xxhash64 path and only
the null-aware loop.
`test_specialised_map_pairs_match_spark` covers all four pairs for **both**
murmur3 and xxhash64,
in two shapes:
- **No nulls:** reaches the fast loop.
- **With nulls:** a null map row that still holds an entry, which must not
be hashed, and a null
value whose key must still be hashed. This reaches the null-aware loop.
The test asserts which loop each shape reaches. The expected values come
from Spark 4.1.3's own
`Murmur3Hash` and `XxHash64` (seed 42), run on the same rows, not from
Comet's output. I checked
that the new test catches bugs the existing tests miss: hashing the value
before the key in the
fast loop, or not skipping null map rows, fails only the new test.
**Inlining / performance.** I built the `hash` benchmark binary in release
mode from the code
before and after this change. The two binaries are byte-identical (same
SHA-256, symbol table
included). So the closures passed to the macro are all inlined, and the
generated code is exactly
what the hand-written arms produced. I did not run timing comparisons, since
identical binaries
would only measure noise. Caveat: the bench reaches only the murmur3 path
(`create_xxhash64_hashes`
is `pub(crate)`), and I checked on one target (x86_64 macOS, rustc 1.99.1).
**Lint.** `cargo fmt --all -- --check` and
`cargo clippy --all-targets --workspace -- -D warnings` are clean.
**Not run locally:** the JVM-side tests (`CometHashExpressionSuite` `hash -
map`,
`sql-tests/expressions/hash/hash.sql`). The change is internal to the native
kernel and produces
an identical murmur3 binary, so I didn't run the Spark SQL or Iceberg suites
either.
**AI assistance:** <!-- in your own words: what you used AI for and what you
checked yourself -->
Claude code helped me to do the baseline and benchmark test before and after
refactor.
--
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]
