You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The measurement you posted on #5397 matches what I'd expect, and I think the reason is worth
writing down so we don't circle back to this later.
On a wide nested schema the hashing cost isn't in the mixing function. It's in create_hashes_internal walking every leaf cell — per-array-type dispatch, recursion into every
struct and list child, null handling — and swapping murmur3 for XXH3 changes only the innermost
and cheapest part of that. XXH3's advantage is on long byte strings, whereas xxh3_64_oneshot
called per cell over 4- and 8-byte primitives lands in its short-input path, which carries more
per-call setup than murmur3's single round. So on exactly the schema shape we're targeting I'd
expect it to come out behind, which is what you saw.
The other half is that placement is only part of the bill. Even with a free hash, routing rows
individually leaves the flush on interleave_record_batch, a per-row gather that re-walks every
column and nested child a second time. #5449 removes the hash and the gather in one move, which
is why it shifts the number by 2x rather than a few percent.
There's also a narrower reason this direction doesn't really have a home. Round robin doesn't
need a content hash at all, which is the premise of #5449, and hash partitioning can't change its
hash — a plan can have one side of a join shuffled natively by Comet and the other by Spark, and
both have to agree on murmur3 with seed 42 or the join silently drops rows. So the two
partitionings that could use a faster mixer are respectively one that shouldn't be hashing and
one that can't change hashes.
Minor, but if we ever do want XXH3 somewhere: native/spark-expr/Cargo.toml:43 already pulls twox-hash 2.1.2, which ships xxhash3_64, so we wouldn't need a second xxhash crate for it.
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
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?
Experiment for #5397 .
Rationale for this change
What changes are included in this PR?
How are these changes tested?