-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add fetch to SortPreservingMergeExec and SortPreservingMergeStream
#6811
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.
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
Changes from all commits
d519a89
2e0ca35
b10c910
1da1f42
878d7a4
38f38de
fd1f2a7
8f3605f
1446162
be015da
193d0ac
99170b9
acaae3e
43d04e0
0a2c8db
ed39a27
19f027f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -189,6 +189,7 @@ impl ExternalSorter { | |
| &self.expr, | ||
| self.metrics.baseline.clone(), | ||
| self.batch_size, | ||
| self.fetch, | ||
| ) | ||
| } else if !self.in_mem_batches.is_empty() { | ||
| let result = self.in_mem_sort_stream(self.metrics.baseline.clone()); | ||
|
|
@@ -285,14 +286,13 @@ impl ExternalSorter { | |
| }) | ||
| .collect::<Result<_>>()?; | ||
|
|
||
| // TODO: Pushdown fetch to streaming merge (#6000) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎉 |
||
|
|
||
| streaming_merge( | ||
| streams, | ||
| self.schema.clone(), | ||
| &self.expr, | ||
| metrics, | ||
| self.batch_size, | ||
| self.fetch, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was removed a few months ago to use |
||
| ) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,7 +67,7 @@ Limit: skip=0, fetch=10 | |
| ------------------TableScan: supplier projection=[s_suppkey, s_comment], partial_filters=[supplier.s_comment LIKE Utf8("%Customer%Complaints%")] | ||
| physical_plan | ||
| GlobalLimitExec: skip=0, fetch=10 | ||
| --SortPreservingMergeExec: [supplier_cnt@3 DESC,p_brand@0 ASC NULLS LAST,p_type@1 ASC NULLS LAST,p_size@2 ASC NULLS LAST] | ||
| --SortPreservingMergeExec: [supplier_cnt@3 DESC,p_brand@0 ASC NULLS LAST,p_type@1 ASC NULLS LAST,p_size@2 ASC NULLS LAST], fetch=10 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
| ----SortExec: fetch=10, expr=[supplier_cnt@3 DESC,p_brand@0 ASC NULLS LAST,p_type@1 ASC NULLS LAST,p_size@2 ASC NULLS LAST] | ||
| ------ProjectionExec: expr=[group_alias_0@0 as part.p_brand, group_alias_1@1 as part.p_type, group_alias_2@2 as part.p_size, COUNT(alias1)@3 as supplier_cnt] | ||
| --------AggregateExec: mode=FinalPartitioned, gby=[group_alias_0@0 as group_alias_0, group_alias_1@1 as group_alias_1, group_alias_2@2 as group_alias_2], aggr=[COUNT(alias1)] | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the hot path for SortPreservingMerge -- I suspect adding an extra check for fetch won't impact performance in any measurable way, but it might be worth while checking the benchmarks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, otherwise we could maybe need two separate implementations (with/without fetch)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As benchmarks showed no significant difference, I kept this check in the hot path.