Skip to content

fix: rebuild the shuffle writer per iteration in end-to-end benches - #6104

Closed
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:fix/shuffle-bench-reexecute
Closed

andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:fix/shuffle-bench-reexecute

Conversation

@andygrove

Copy link
Copy Markdown
Member

Which issue does this PR close?

No issue filed. Reported in #6095 (comment).

Rationale for this change

Every end-to-end bench in native/shuffle/benches/shuffle_writer.rs panics during criterion warmup:

shuffle write error: partition offsets were already published

Each bench builds one ShuffleWriterExec outside b.iter() and executes it on every iteration, but the writer publishes its partition offsets through PartitionOffsets, a OnceLock that errors on a second set. CI does not catch this because pr_benchmark_check.yml only runs cargo check --benches.

What changes are included in this PR?

  • A bench_end_to_end helper that uses b.iter_batched to build a fresh exec for each iteration. Construction counts as setup and is not timed.
  • The four end-to-end bench loops now call that helper instead of repeating the same execute block.

How are these changes tested?

Ran cargo bench -p datafusion-comet-shuffle --bench shuffle_writer -- --quick "end to end". All 12 end-to-end benches now finish (compression variants, hash/range/single partitioning, and 200/2000/8000 partitions). Before this change, all of them panicked.

A ShuffleWriterExec publishes its partition offsets through a OnceLock,
so re-executing one exec across criterion iterations fails on the second
run with "partition offsets were already published". Build a fresh exec
per iteration with iter_batched, keeping construction out of the timing.
@github-actions github-actions Bot added bug Something isn't working area:shuffle Shuffle (JVM and native) labels Sep 22, 2026
@andygrove

Copy link
Copy Markdown
Member Author

Folded this into #6095 instead, as the last commit on that branch. That PR cannot be benchmarked
at all until this lands, so splitting them just means measuring #6095 against a branch that has to
carry this anyway. The change is unchanged from what was here.

Closing in favour of #6095.

@andygrove andygrove closed this Sep 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:shuffle Shuffle (JVM and native) bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant