Add RowSelection::from_selectors_and_combine to merge RowSelectors #2994
Add RowSelection::from_selectors_and_combine to merge RowSelectors #2994alamb merged 5 commits intoapache:masterfrom
RowSelection::from_selectors_and_combine to merge RowSelectors #2994Conversation
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Look good to me @Ted-Jiang -- nice work! I have one more set of tests I recommend, but otherwise this PR looks good to me 👍 thank you!
cc @tustvold
| Self { selectors } | ||
| } | ||
|
|
||
| /// Creates a [`RowSelection`] from a slice of uncombined `RowSelector`: |
| } | ||
| let first = selectors.first().unwrap(); | ||
| let mut sum_rows = first.row_count; | ||
| let mut selected = first.skip; |
There was a problem hiding this comment.
| let mut selected = first.skip; | |
| let mut skip = first.skip; |
Maybe it would be easier to read this code if the name of the variable is the same as the name of the field that uses it 🤔
Co-authored-by: Andrew Lamb <[email protected]>
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Looks great -- thanks @Ted-Jiang
alamb
changed the title
RowSelection::from_selectors_and_combine to merge RowSelectors
|
Benchmark runs are scheduled for baseline = 62e878e and contender = f11372c. f11372c is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
3 participants
Which issue does this PR close?
Closes #2858 .
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?