viirya opened a new pull request, #5757:
URL: https://github.com/apache/datafusion-comet/pull/5757
## Which issue does this PR close?
Closes #5756.
## Rationale for this change
The dictionary fast path hashes each distinct dictionary value once and
reuses that result
for every key. It was selected by the column's position and restarted from a
hardcoded seed:
```rust
let first_col = i == 0;
...
if !first_col { /* unpack and recurse */ } else {
let mut dict_hashes = vec![42; dict_values.len()];
```
Two things are wrong with that:
- `create_hashes_internal!` also runs on **recursion**, so a dictionary
nested in a list,
struct or map arrives as the only column of its call and looks like a
first column, even
though the buffer already holds the hash accumulated for earlier elements
of that row. That
hash was discarded, so a dictionary-encoded list element hashed
differently from the
identical decoded value.
- The **hardcoded 42** is wrong whenever the caller supplies its own seed,
which
`hash(col, seed)` and `xxhash64(col, seed)` allow, so even a genuine first
column disagreed
with its decoded form for a non-default seed.
For a shuffle partitioning key that means equal keys can reach different
partitions, breaking
grouping and joins; the `hash()` and `xxhash64()` SQL functions are affected
too.
## What changes are included in this PR?
- The reuse is valid exactly when every row carries the same incoming hash,
so that is what
is checked now, rather than the column index.
- The per-value hashes start from the seed the buffer actually holds instead
of an assumed 42.
- Both changes are in murmur3 and xxhash64, which share this structure.
A top-level dictionary column keeps the optimisation.
**On the cost of the check.** It is a scan of the hash buffer, and that is
not free: running
it for every column measured 17% slower on an int column and 11% on a string
column in a
local criterion benchmark. It is therefore done inside the dictionary arm,
so only dictionary
columns pay for it. After moving it, the same benchmark is back at the
unmodified timings.
## How are these changes tested?
`cargo test -p datafusion-comet-spark-expr` passes 713 + 5 tests.
Three new tests, all failing without the change:
- a dictionary as a list element, compared against the decoded array —
hashes `3853467749`
instead of `1401423033` before the fix
- the same for xxhash64
- a top-level dictionary column for both seed 42 and seed 7, which pins that
the fast path
survives and that a non-default seed is handled
## Additional context
Found while reviewing nested hash partitioning keys (#5567), which makes
this reachable from
shuffle partitioning, but the defect predates it and reproduces on `main`
unchanged.
Worth recording how it was found, since it says something about the tests:
comparing a batched
hash against a per-row hash cannot catch this, because both sides run the
same faulty branch.
It took an independent leaf-by-leaf chaining oracle to surface it. Credit to
Codex for that
approach.
--
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]
