Skip to content

bench: add sort benchmarks for various data profile - #23346

Merged
rluvaton merged 7 commits into
apache:mainfrom
rluvaton:add-sort-benchmarks
Jul 11, 2026
Merged

bench: add sort benchmarks for various data profile#23346
rluvaton merged 7 commits into
apache:mainfrom
rluvaton:add-sort-benchmarks

Conversation

@rluvaton

Copy link
Copy Markdown
Member

Which issue does this PR close?

N/A

Rationale for this change

I'm adding multiple sort optimization in the coming PRs and I need a benchmark to compare against main

What changes are included in this PR?

added benchmark for various data profiles

Are these changes tested?

N/A

Are there any user-facing changes?

Nope

@kosiewkosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rluvaton
Looks good overall.

I left a couple of small benchmark readability and measurement suggestions.

c.bench_function(
&format!("spm/{ordering}/{key_type}/payload_{payload_width}"),
|b| {
b.iter(|| {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition. One small thought: this benchmark builds TestMemoryExec and SortPreservingMergeExec inside b.iter. The setup cost is probably small compared with draining 400k rows, but moving it out of the timed section would make optimizer and runtime comparisons a bit cleaner.

Could we use iter_batched, like the benchmark above, so the measured part focuses more directly on stream execution and drain?

match ordering {
"sorted" => (0..n).collect(),
"reverse" => (0..n).rev().collect(),
// Sorted except for ~1% of items misplaced to random positions

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, but I think a short comment would help future readers interpret the benchmark correctly. Since the nearly_sorted disorder is introduced before round-robin batch partitioning and per-partition sorting, SPM is seeing sorted input partitions whose membership came from a near-sorted arrival stream, rather than directly nearly-sorted partitions.

@github-actionsgithub-actionsBot added core Core DataFusion crate and removed physical-plan Changes to the physical-plan crate labels Jul 9, 2026
@rluvaton

Copy link
Copy Markdown
MemberAuthor

@kosiew Can you please re-review - I moved the benchmarks and benchmarked SortExec instead

since having nearly sorted while have value is not what I meant since I forgot we sort before passing to SortPreservingMergeExec

@rluvaton
rluvaton added this pull request to the merge queueJul 11, 2026
Merged via the queue into apache:main with commit 9ec28bdJul 11, 2026
36 checks passed
@rluvaton
rluvaton deleted the add-sort-benchmarks branch July 11, 2026 17:47
@rluvaton

Copy link
Copy Markdown
MemberAuthor

I might created benchmarks that takes too much time 😅

kosiew pushed a commit to kosiew/datafusion that referenced this pull request Aug 12, 2026
## Which issue does this PR close?
N/A
## Rationale for this change
I'm adding multiple sort optimization in the coming PRs and I need a
benchmark to compare against main
## What changes are included in this PR?
added benchmark for various data profiles
## Are these changes tested?
N/A
## Are there any user-facing changes?
Nope
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

coreCore DataFusion crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@rluvaton@kosiew