fix: keep the dictionary hash fast path off nested and reseeded buffers - #5757
Open
viirya wants to merge 1 commit into
Open
fix: keep the dictionary hash fast path off nested and reseeded buffers#5757viirya wants to merge 1 commit into
viirya wants to merge 1 commit into
Conversation
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, and a dictionary-encoded list element hashed differently from the identical decoded value. Separately, the hardcoded 42 is wrong whenever the caller supplies its own seed, as `hash(col, seed)` and `xxhash64(col, seed)` allow, so even a genuine first column disagreed with its decoded form for a non-default seed. The reuse is valid exactly when every row carries the same incoming hash, so that is what is now checked, and the per-value hashes start from the seed the buffer actually holds rather than an assumed 42. A top-level dictionary keeps the optimisation. The uniformity check is a scan of the hash buffer, which is measurable: running it for every column cost 17% 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 it and other types are untouched. Both hash implementations share this structure and both are fixed, with regression tests that fail without the change: a dictionary as a list element hashes 3853467749 rather than the 1401423033 of the decoded data. The single-row cases above pin the hardcoded seed but not the uniformity check, since one row is uniform by definition. Each algorithm therefore also gets a multi-row case whose incoming seeds all differ, which forces the unpacking fallback, and which includes a null key and a key pointing at a null dictionary value. Dropping the uniformity check leaves the other twenty hash tests green and fails exactly those two. Co-authored-by: Claude Code <noreply@anthropic.com>
viirya
force-pushed
the
fix-dict-hash-nested-seed
branch
from
September 7, 2026 07:30
e0ee150 to
b53ebc1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
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.
hash(col, seed)andxxhash64(col, seed)allow, so even a genuine first column disagreedwith 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()andxxhash64()SQL functions are affected too.What changes are included in this PR?
is checked now, rather than the column index.
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-exprpasses 713 + 5 tests.Three new tests, all failing without the change:
3853467749instead of
1401423033before the fixsurvives 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
mainunchanged.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.