Repository navigation
GH-51156: [C++] Raise CapacityError instead of truncating binary values over 2 GiB - #51158
Merged
Merged
Conversation
…y values over 2 GiB
|
|
Contributor
|
@ngoldbaum, sorry for the confusion—I made a mistake. I was referring to a different topic. |
pitrou
added
backport-candidate
Critical Fix
pitrou
approved these changes
Sep 7, 2026
pitrou
left a comment
pitrou
left a comment
Member
There was a problem hiding this comment.
+1, thanks for the fix @ngoldbaum !
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
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
BaseBinaryBuilder::Append(std::string_view),ExtendCurrent(std::string_view)andChunkedBinaryBuilder::Append(std::string_view)cast the value size toint32_tbefore validating it, so a value of 2 GiB or more is stored as an empty or truncated value. See #51156.What changes are included in this PR?
The three overloads validate the 64-bit size before narrowing it, returning the same
CapacityErrorthatValidateOverflowproduces for the other paths.Are these changes tested?
Yes, two
LARGE_MEMORY_TESTcases inarray_binary_test.cc. They fail without the fix.Are there any user-facing changes?
BinaryBuilder,StringBuilderandChunkedBinaryBuildernow returnCapacityErrorfor astring_viewvalue of 2 GiB or more instead of storing wrong data.This PR contains a "Critical Fix". The builders produced incorrect data for such values.
AI disclosure: I used an AI model to think about and iterate on the fix. It also originally identified the bug.