Skip to content

fix: keep the dictionary hash fast path off nested and reseeded buffers - #5757

Open
viirya wants to merge 1 commit into
apache:mainfrom
viirya:fix-dict-hash-nested-seed
Open

fix: keep the dictionary hash fast path off nested and reseeded buffers#5757
viirya wants to merge 1 commit into
apache:mainfrom
viirya:fix-dict-hash-nested-seed

Conversation

@viirya

@viirya viirya commented Sep 7, 2026

Copy link
Copy Markdown
Member

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:

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.

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
viirya force-pushed the fix-dict-hash-nested-seed branch from e0ee150 to b53ebc1 Compare September 7, 2026 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dictionary-encoded values hash differently from decoded values inside nested types

1 participant