parquet: skip heap allocation on terminal page skip in DeltaBitPackDecoder#9787
Closed
sahuagin wants to merge 4 commits intoapache:mainfrom
Closed
parquet: skip heap allocation on terminal page skip in DeltaBitPackDecoder#9787sahuagin wants to merge 4 commits intoapache:mainfrom
sahuagin wants to merge 4 commits intoapache:mainfrom
Conversation
…PackDecoder When to_skip >= values_left the caller is discarding all remaining values on the page and last_value will not be read again. In this case use BitReader::skip(n, bit_width) to advance the stream without decoding, and return early without allocating a scratch buffer or updating last_value. The existing decode path is preserved unchanged for non-terminal skips where last_value accuracy is required for subsequent get() calls. Split from apache#9769 as requested by reviewer.
etseidl
reviewed
Apr 21, 2026
| // Terminal skip: caller is discarding all remaining values on this page. | ||
| // last_value will never be read again, so we can use O(1) arithmetic | ||
| // skips (BitReader::skip) instead of decoding through get_batch. | ||
| let terminal = to_skip >= self.values_left + skip; |
Contributor
There was a problem hiding this comment.
I think this line and the following block can move before the preceding block...then this test can just be to_skip >= self.values_left.
Contributor
Author
There was a problem hiding this comment.
I can take a look a bit later this evening.
| let terminal = to_skip >= self.values_left + skip; | ||
|
|
||
| if terminal { | ||
| while skip < to_skip { |
Contributor
There was a problem hiding this comment.
Since we're skipping the entire page, I think we can skip all of this logic and simply set self.values_left to 0. The bit reader is populated with a single page worth of data, so it's safe to not run through the state machine here.
This suggestion messes with one of the unit tests, but the test can be modified to not skip the entire page and still trigger the expected error.
Contributor
Author
There was a problem hiding this comment.
I have some concerns changing the tests to match my code: I'll call them out specifically in my changes to make sure they are the ones that you were thinking of.
Contributor
|
run benchmark arrow_reader |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing pr/terminal-skip (0ee64e0) to de11d9c (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
…pressure on non-terminal path
Contributor
|
run benchmark arrow_reader |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing pr/terminal-skip (1a8cdf3) to de11d9c (merge-base) diff File an issue against this benchmark runner |
Contributor
Author
|
Note on benchmark variance: These results were collected on a non-isolated machine without CPU frequency pinning. Small variances of ±5% on non-bw=0 paths (particularly |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Two changes from etseidl review on apache#9787: 1. Move the terminal-skip check before first_value consumption. The predicate simplifies to `to_skip >= self.values_left` (no need to account for a just-consumed first_value), and in the terminal case we never needed to read first_value anyway since it's being discarded. 2. Since we're discarding the entire page, there is no need to walk the miniblock state machine at all. The bit reader holds only one page worth of data and any subsequent decoder work goes through set_data() for a fresh page, so we can simply zero self.values_left and return. This removes the #[cold] helper entirely — the terminal path is now three lines inline. The invalid-bit-width test now needs skip(31) instead of skip(32) so the non-terminal path exercises check_bit_width and triggers the expected error. Net: -26 / +15 lines; #[cold] helper and its icache-pressure workaround no longer needed.
Contributor
|
I finally remembered why the optimization in this PR is not showing up in the benchmarks... // If page has less rows than the remaining records to
// be skipped, skip entire page
let rows = metadata.num_rows.or_else(|| {
// If no repetition levels, num_levels == num_rows
self.rep_level_decoder
.is_none()
.then_some(metadata.num_levels)?
});
if let Some(rows) = rows {
if rows <= remaining_records {
self.page_reader.skip_next_page()?;
remaining_records -= rows;
continue;
}
}
Take that back...farther down there's if self.num_buffered_values == self.num_decoded_values {
// Exhausted buffered page - no need to advance other decoders
continue;
}which will catch exhausted pages with repetition. TL;DR I think we can close this PR. |
Contributor
Author
|
Here are the updated benchmark results on the simplified terminal-skip branch against the SummaryThe targeted paths — bw=0 miniblocks and terminal page skips — show consistent improvements on the 8/16/32-bit integer types. Results on 64-bit types are mixed, likely dominated by measurement variance (see note at the end). Wins on the bw=0 / terminal-skip paths (8/16/32-bit)
Noise on unchanged pathsThe Note on 64-bit varianceInt64Array/UInt64Array results on this run were noisier than the smaller types, including +6.3% / +2.1% on Happy to rerun on more controlled hardware if that'd be useful. Measurement conditionsSame disclaimer as the prior run: non-isolated machine, no CPU frequency pinning, browser tabs and assorted background processes. For the paths I targeted the signal is large enough to read through the noise; for paths I didn't touch I'd ignore the numbers on principle. |
Contributor
|
As a test, I replaced the contents of Thanks for the effort, but I don't think there is any win to be had here. |
Contributor
Author
|
Thanks @etseidl — the panic test is conclusive. I traced
The structural refactor from the earlier round of review (moving the fast-path before Closing. |
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.
Adds a terminal-skip fast path to
DeltaBitPackDecoder::skip(). Whento_skip >= values_leftthe caller is discarding all remaining values on thepage and
last_valuewill not be read again. In this case:BitReader::skip(n, bit_width)to advance the stream without decoding.last_value.The terminal path is extracted into a
#[cold]helper to prevent the addedcode from increasing the icache footprint of the non-terminal hot path.
The existing decode path is preserved unchanged for non-terminal skips where
last_valueaccuracy is required for subsequentget()calls.Benchmarks (
arrow_readerbench vs upstream HEAD,-- skip --baseline upstream):No regressions on non-terminal paths. Benchmarks were run on a non-isolated
machine (no CPU frequency pinning); small variances of ±5% should be attributed
to measurement noise.
Origin: The common "skip rest of page" pattern during row-group filtering
hits this path frequently. Removing the unconditional scratch allocation for a
case where the decoded values are immediately discarded is the right call.
Verification: Existing skip tests pass. One new focused test added:
decode partial values first, then terminal-skip the remainder — the real-world
row-filtering pattern that was not previously covered.
Generated-by: Claude (claude-sonnet-4-6)