Skip to content

feat: RR partition with xxhash-rust - #5450

Closed
comphead wants to merge 1 commit into
apache:mainfrom
comphead:shuffle_writes
Closed

comphead wants to merge 1 commit into
apache:mainfrom
comphead:shuffle_writes

Conversation

@comphead

@comphead comphead commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

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?

@andygrove

Copy link
Copy Markdown
Member

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.

Shall we close this in favour of #5449?

@comphead

Copy link
Copy Markdown
Contributor Author

Thanks @andygrove closing it.

@comphead comphead closed this Aug 30, 2026
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.

2 participants