andygrove opened a new pull request, #5817:
URL: https://github.com/apache/datafusion-comet/pull/5817
Backport of #5757 to `branch-1.0`.
Cherry-picked from `92ad99e97482c861062f52372b172320197c2001` with no
conflicts and no
modifications — the diff is byte-identical to the original PR.
## Which issue does this PR close?
Closes #5756 on `branch-1.0`.
## 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, `i == 0`, and it
restarted from a hardcoded
seed of 42. Both parts are wrong:
- `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. The uniformity check
is a scan of the hash
buffer and is measurable, so it is done inside the dictionary arm — only
dictionary columns pay
for it.
## How are these changes tested?
Same tests as the original PR, verified locally on `branch-1.0`:
- `cargo test -p datafusion-comet-spark-expr` passes 615 + 3 tests.
- `cargo check --workspace --all-targets` is clean across all six crates, and
`cargo clippy -p datafusion-comet-spark-expr --all-targets -- -D warnings`
and
`cargo fmt --all -- --check` pass.
- The five new regression tests were confirmed to fail on `branch-1.0` with
the fix reverted and
the tests kept, reproducing the exact values from the original PR: the
dictionary list element
hashes `3853467749` instead of `1401423033`, and the top-level dictionary
test fails on the
seed-7 case. So the defect is present on `branch-1.0` and this backport is
what removes it.
`branch-1.0` pins the same DataFusion version (54.1.0) as `main`, so no API
adaptation was needed.
## Are there any user-facing changes?
Dictionary-encoded values now hash identically to their decoded form inside
nested types and for
non-default seeds. Hash values for those cases change, which is the point of
the fix — a
top-level dictionary column with the default seed is unaffected.
--
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]
