parquet: rle skip decode loop when batch contains all max levels (aka no nulls)#9258
Merged
alamb merged 2 commits intoapache:mainfrom Feb 5, 2026
Merged
parquet: rle skip decode loop when batch contains all max levels (aka no nulls)#9258alamb merged 2 commits intoapache:mainfrom
alamb merged 2 commits intoapache:mainfrom
Conversation
lyang24
force-pushed
the
rle_fast_path
branch
from
24968e2 to
58bfbcc
Compare
lyang24
force-pushed
the
rle_fast_path
branch
from
58bfbcc to
f5a1966
Compare
lyang24
changed the title
Contributor
|
run benchmark arrow_reader |
|
🤖 |
|
🤖: Benchmark completed Details
|
lyang24
changed the title
Contributor
|
run benchmark arrow_reader |
|
🤖 |
alamb
reviewed
Jan 30, 2026
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
These look like very promising results -- thank you @lyang24 🙏
I studied this fairly carefully and it makes sense to me (and it made sense to codex too). I'll study the performance results and assuming those look good I think this one is good to merge in
cc @tustvold and @Dandandan in case you have time to help review this
| /// Returns `Ok(None)` if there are any nulls or packed data that prevents fast path. | ||
| /// | ||
| /// On success, advances decoder state. On failure, state is unchanged. | ||
| fn try_skip_all_valid(&mut self, len: usize) -> Result<Option<usize>> { |
Contributor
There was a problem hiding this comment.
I found this name confusing as it doesn't really "skip" the levels (they are returned in the count)
What would you think about renaming this try_consume_all_valid instead?
Contributor
Author
There was a problem hiding this comment.
yep try_consume_all_valid fits better i have updated it in the second commit
| /// Returns `Ok(Some(count))` if successfully consumed `count` all-valid levels. | ||
| /// Returns `Ok(None)` if there are any nulls or packed data that prevents fast path. | ||
| /// | ||
| /// On success, advances decoder state. On failure, state is unchanged. |
Contributor
There was a problem hiding this comment.
technically speaking the state is changed (block can be advanced) but that only happens if rle_left was zero (and hence it would have been loaded anyways)
Contributor
Author
There was a problem hiding this comment.
good call updated comments
alamb
changed the title
|
🤖: Benchmark completed Details
|
alamb
approved these changes
Jan 30, 2026
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
I think this is a really nice find
Contributor
|
Let's do it (I have been procrastinating because I have some nagging feeling that there is a corner case here or something) However, I spent quite a while with codex trying to tease a bug out and I could no Thanks again @lyang24 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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
parquet reading perf - if rle value is true and rle left have enough room for the current batch. lets skip the decode loop the overhead of count_set_bits for null bitmap.
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?