[parquet] Avoid a clone while resolving the read strategy #9056
+13
−10
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alamb
changed the title
alamb
commented
Dec 28, 2025
| None => return RowSelectionStrategy::Selectors, | ||
| }; | ||
|
|
||
| let trimmed = selection.clone().trim(); |
Contributor
Author
There was a problem hiding this comment.
this clone shows up in the traces for some of the clickbench queries. I think this is because there are 9000 selectors for these queries
hhhizzz
reviewed
Dec 29, 2025
| let total_rows: usize = selectors.iter().map(|s| s.row_count).sum(); | ||
| let selector_count = selectors.len(); | ||
| if selector_count == 0 { | ||
| let non_empty_selector_count = selection.iter().filter(|s| s.row_count > 0).count(); |
Contributor
There was a problem hiding this comment.
Good improvement! I didn’t notice this in my benchmarks, likely because the dataset was too small.
Small suggestion:
non_empty_selector_countis too long,effective_countcould sound better.- Using
foldto replace 2 seperate iteration might be better.
let (total_rows, effective_count) = selection.iter()
.fold((0, 0), |(rows, count), s| {
if s.row_count > 0 {
(rows + s.row_count, count + 1)
} else {
(rows, count)
}
});
if effective_count == 0 {
return RowSelectionStrategy::Mask;
}
if total_rows < effective_count.saturating_mul(threshold) {
RowSelectionStrategy::Mask
} else {
RowSelectionStrategy::Selectors
}
6 tasks
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
run benchmark arrow_reader_row_filter |
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
@hhhizzz do you think this PR is ready to merge? |
Contributor
Yeah, I think its ready to merge. |
Contributor
Author
Thank you for the review ❤️ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
While working on apache/datafusion#19477, and profiling ClickBench q7, I noticed that the RowSelectors was being cloned to resolve the strategy -- for a large number of selections this is expensive and shows up in the traces
We should change the code to avoid cloning the RowSelectors when resolving the strategy.
Changes
Don't clone / allocate while resolving the strategy.
I don't expect this to have a massive impact, but it did show up in the profile
FYI @hhhizzz -- perhaps you could review this PR
Are these changes tested?
Yes by CI
Are there any user-facing changes?
small performance improvement