Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Parallel merge sort (#6162) #6308

Merged
merged 3 commits into from
May 11, 2023
Merged

Parallel merge sort (#6162) #6308

merged 3 commits into from
May 11, 2023

Conversation

tustvold
Copy link
Contributor

@tustvold tustvold commented May 9, 2023

Which issue does this PR close?

Closes #6162

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Will run benchmarks prior to merge

Are there any user-facing changes?

@github-actions github-actions bot added the core Core DataFusion crate label May 9, 2023
@alamb
Copy link
Contributor

alamb commented May 10, 2023

I plan to review this carefully tomorrow morning

alamb
alamb approved these changes May 10, 2023
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

The code looks great. Thank you @tustvold

I agree some benchmarks are in order but I think the code itself is great.

datafusion/core/src/physical_plan/common.rs
mut input: SendableRecordBatchStream,
buffer: usize,
) -> SendableRecordBatchStream {
// Use tokio only if running from a tokio context (#2201)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

datafusion/core/src/physical_plan/sorts/sort_preserving_merge.rs
.map(|partition| self.input.execute(partition, context.clone()))
.collect::<Result<_>>()?,
};
let receivers = (0..input_partitions)
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks beautiful

@tustvold
Copy link
Contributor Author

It isn't a huge win, but I'll take it

sort utf8 low cardinality
                        time:   [9.0271 ms 9.0396 ms 9.0527 ms]
                        change: [-11.727% -11.548% -11.370%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
  3 (3.00%) high mild
  1 (1.00%) high severe
merge sorted utf8 high cardinality
                        time:   [7.8055 ms 7.8186 ms 7.8341 ms]
                        change: [-3.5327% -3.0466% -2.5856%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
  2 (2.00%) high mild
  2 (2.00%) high severe

sort merge utf8 high cardinality
                        time:   [8.0414 ms 8.0505 ms 8.0599 ms]
                        change: [-2.5665% -2.3550% -2.1393%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild

sort utf8 high cardinality
                        time:   [12.386 ms 12.413 ms 12.442 ms]
                        change: [-15.231% -14.371% -13.530%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
  7 (7.00%) high mild
  1 (1.00%) high severe
sort utf8 tuple         time:   [19.995 ms 20.037 ms 20.082 ms]
                        change: [-23.549% -23.240% -22.940%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
  1 (1.00%) high mild
  3 (3.00%) high severe
sort mixed dictionary tuple
                        time:   [21.851 ms 21.939 ms 22.033 ms]
                        change: [-13.963% -13.555% -13.128%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
  6 (6.00%) high mild
sort mixed tuple        time:   [19.054 ms 19.176 ms 19.307 ms]
                        change: [-13.179% -12.578% -11.928%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 12 outliers among 100 measurements (12.00%)
  11 (11.00%) high mild
  1 (1.00%) high severe

There is no change the merge benches and the benches for primitive arrays (as they don't use SortPreservingMerge)

@alamb
Copy link
Contributor

alamb commented May 10, 2023

I think we should merge it

@tustvold tustvold merged commit 68061aa into apache:main May 11, 2023
@gruuya gruuya mentioned this pull request Aug 2, 2023
Closed
@Kontinuation Kontinuation mentioned this pull request Feb 15, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
core Core DataFusion crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Run SortPreservingMerge Inputs Concurrently
2 participants
@tustvold @alamb