POC: Avoid a copy for uncompressed pages #8756
Closed
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
commented
Oct 31, 2025
| let decompressed_size = uncompressed_page_size - offset; | ||
| let mut decompressed = Vec::with_capacity(uncompressed_page_size); | ||
| decompressed.extend_from_slice(&buffer.as_ref()[..offset]); | ||
| if decompressed_size > 0 { |
Contributor
Author
There was a problem hiding this comment.
You can see here that if decompressed_size is 0, it returns decompressed unmodified and decompressed was just the first offset bytes of the input buffer
I changed the code to return buffer.slice(..offset) in this case
I can add it to this location directly to make the diff smaller, but the logic was already hard to follow and highly indented to I moved it into its own function while I was at it
|
|
||
| let decompressed_size = uncompressed_page_size - offset; | ||
|
|
||
| if decompressed_size == 0 { |
etseidl
reviewed
Oct 31, 2025
Comment on lines
390
to
391
| // TODO: page header could be huge because of statistics. We should set a | ||
| // maximum page header size and abort if that is exceeded. |
Contributor
There was a problem hiding this comment.
Not relevant to this PR, but I think this TODO has largely been addressed by #8376 which enabled skipping the decoding of the page statistics.
mapleFU
reviewed
Oct 31, 2025
Comment on lines
+459
to
+460
| /// the provided decompressor if available and applicable. If the buffer is not | ||
| /// compressed, it will be returned as is. |
Member
There was a problem hiding this comment.
The code looks good to me but the I don't know if the comment "not compressed" can be replaced, if decompress_buffer is called and decompressed_size == 0 , seems that it generally means something like "this page only have levels, but not have non-null values"? ( Point me out if I'm wrong)
Contributor
Author
There was a problem hiding this comment.
I actually also do not know what decompressed_size really means -- you are probably right. I will research it more carefully
Contributor
Author
There was a problem hiding this comment.
I read https://github.com/apache/parquet-format/blob/master/README.md#data-pages and I agree with your assessment that if the decompressed_size=0 it corresponds to no non-null values
Contributor
Author
|
🤖 |
Contributor
Author
|
🤖: Benchmark completed Details
|
Contributor
Author
|
🤖 |
Contributor
Author
|
🤖: Benchmark completed Details
|
Contributor
Author
|
🤖 |
Contributor
Author
|
🤖: Benchmark completed Details
|
Contributor
Author
|
If anything the benchmark results show a small, but consistent slowdown with this version |
alamb
added a commit
that referenced
this pull request
Nov 6, 2025
# Which issue does this PR close? - follow on to #8756 # Rationale for this change @etseidl comments: #8756 (comment) > Not relevant to this PR, but I think this TODO has largely been addressed by #8376 which enabled skipping the decoding of the page statistics. While I was in here, I also wanted to capture the learning based on @mapleFU 's comment #8756 (comment) > The code looks good to me but the I don't know if the comment "not compressed" can be replaced, if decompress_buffer is called and decompressed_size == 0 , seems that it generally means something like "this page only have levels, but not have non-null values"? ( Point me out if I'm wrong) # What changes are included in this PR? Include some comments # Are these changes tested? No (there are no code changes) # Are there any user-facing changes? No, this is internal comments only. No code / behavior changes
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?
SerializedPageReader#8745Rationale for this change
While reviewing #8745 I noticed the page was always copied into a new allocation even when there is at least one path that doesn't use it: #8745 (comment)
What changes are included in this PR?
Are these changes tested?
By CI, and I will run benchmarks
I also ran code coverage in the tests to see if this code path (decompressed_size == 0) is hit often:
It does appear to happen, but not often in the tests (happens 3 times in the tests)
Are there any user-facing changes?
No