GH-49959: [C++][Parquet] Avoid unbounded temp alloc in BYTE_STREAM_SPLIT decoder#49960
Merged
Merged
Conversation
github-actions
Bot
added
awaiting review
pitrou
force-pushed
the
gh49959-bss-temp-mem-alloc
branch
from
May 11, 2026 14:08
dd92425 to
14e16db
Compare
Member
Author
|
@github-actions crossbow submit -g cpp |
|
Revision: 14e16db Submitted crossbow builds: ursacomputing/crossbow @ actions-51f44d0cee
|
pitrou
marked this pull request as ready for review
May 11, 2026 14:52
pitrou
force-pushed
the
gh49959-bss-temp-mem-alloc
branch
from
May 12, 2026 07:03
14e16db to
9a3d8cc
Compare
wgtmac
approved these changes
May 12, 2026
github-actions
Bot
added
awaiting committer review
…EAM_SPLIT decoder
pitrou
force-pushed
the
gh49959-bss-temp-mem-alloc
branch
from
May 12, 2026 09:21
9a3d8cc to
982682b
Compare
Member
Author
|
Thanks for the review @wgtmac ! |
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit a3f3d8b. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them. |
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit a3f3d8b. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Mottl
pushed a commit
to Mottl/arrow
that referenced
this pull request
May 26, 2026
…EAM_SPLIT decoder (apache#49960) ### Rationale for this change The BYTE_STREAM_SPLIT encoder and decoder allocate a `SmallVector` to hold the addresses of the different streams. While this is valid for small number of streams (which is most or all legitimate use cases), very large numbers of streams (e.g. FLBA(100000000)) can trigger a huge temporary memory allocation. This issue was found by OSS-Fuzz: https://issues.oss-fuzz.com/issues/511575321 ### What changes are included in this PR? 1. When the width of the BYTE_STREAM_SPLIT-encoded type is larger than a predefined constant, switch to a slower implementation that doesn't need any temporary allocation 2. When encoding or decoding 0 values, avoid some pointless setup work 3. Add tests for the two conditions above ### Are these changes tested? Yes, by additional unit tests and by additional fuzz regression file. ### Are there any user-facing changes? * GitHub Issue: apache#49959 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
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
2 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.
Rationale for this change
The BYTE_STREAM_SPLIT encoder and decoder allocate a
SmallVectorto hold the addresses of the different streams. While this is valid for small number of streams (which is most or all legitimate use cases), very large numbers of streams (e.g. FLBA(100000000)) can trigger a huge temporary memory allocation.This issue was found by OSS-Fuzz: https://issues.oss-fuzz.com/issues/511575321
What changes are included in this PR?
Are these changes tested?
Yes, by additional unit tests and by additional fuzz regression file.
Are there any user-facing changes?