Use fetch limit in get_sorted_iter
#3545
Conversation
| @@ -273,6 +275,7 @@ impl MemoryConsumer for ExternalSorter { | |||
| &self.expr, | |||
| self.session_config.batch_size(), | |||
| tracking_metrics, | |||
| self.fetch, | |||
There was a problem hiding this comment.
Nice thing is that it also reduces disk spilling, as sort + limit is done before writing.
There was a problem hiding this comment.
Although to be honest, I would hope that if there is a LIMIT on the query we could probably avoid the spilling entirely
There was a problem hiding this comment.
Yeah maybe the spilling could see the remaining batch is so small it could add the sorted data to memory again - avoiding the spill 🤔
There was a problem hiding this comment.
As a future PR / optimization perhaps
| .map(|i| row_indices[*i as usize]) | ||
| .collect(); | ||
|
|
||
| Ok(SortedIterator::new(row_indices, batch_size)) |
There was a problem hiding this comment.
Some cleanup - we can do this immediately instead of keeping it in SortedIterator
| // distributed with this work for additional information | ||
| // regarding copyright ownership. The ASF licenses this file | ||
| // to you under the Apache License, Version 2.0 (the | ||
| // "License"); you may not use this file except in compliance |
There was a problem hiding this comment.
Moved this to planner - seems a bit more simple. We don't need access to the parent anymore now.
There was a problem hiding this comment.
This looks like a great improvement -- nice work @Dandandan
| @@ -273,6 +275,7 @@ impl MemoryConsumer for ExternalSorter { | |||
| &self.expr, | |||
| self.session_config.batch_size(), | |||
| tracking_metrics, | |||
| self.fetch, | |||
There was a problem hiding this comment.
Although to be honest, I would hope that if there is a LIMIT on the query we could probably avoid the spilling entirely
| @@ -374,44 +379,38 @@ fn get_sorted_iter( | |||
| }) | |||
| }) | |||
| .collect::<Result<Vec<_>>>()?; | |||
| let indices = lexsort_to_indices(&sort_columns, None)?; | |||
| let indices = lexsort_to_indices(&sort_columns, fetch)?; | |||
| /// Indexes into the input representing the correctly sorted total output | ||
| indices: UInt32Array, | ||
| /// Map each each logical input index to where it can be found in the sorted input batches | ||
| /// Sorted composite index of where to find the rows in buffered batches |
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
|
Benchmark runs are scheduled for baseline = 0a2b0a7 and contender = ff718d0. ff718d0 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
This PR appears to have broken the build -- PR to fix: #3576 |
Which issue does this PR close?
Closes #3544
Rationale for this change
Provides a small speedup vs the earlier results.
We can see from the output from
explain analyze select l_orderkey from t order by l_orderkey limit 10;Before:
After(note, only 160 rows=10 rows * 16 partitions)
What changes are included in this PR?
Are there any user-facing changes?