Repository navigation
GH-55606: [C++] Detect overflow when parsing decimals from strings - #55619
Open
csarushan1729 wants to merge 1 commit into
Open
csarushan1729 wants to merge 1 commit into
csarushan1729 wants to merge 1 commit into
Conversation
|
|
|
|
|
|
1 similar comment
|
|
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
Decimal{32,64,128,256}::FromStringbuilds the absolute value in fixed-width words without checking for overflow. Values wider than the storage integer wrap modulo 2^width, and the cast kernel'sFitsInPrecision()check then passes on the wrapped value. As a result,cast(string -> decimal)(and anything else built onFromString, such as the CSV reader) silently returns wrong values:"9" * 392**642**256 + 72**128 - 1"99e38""9e9"There are three ways this can wrap:
ShiftAndAdddrops the carry out of the top word).*out *= GetScaleMultiplier(...)is unchecked).[2^(N-1), 2^N)for 128/256-bit decimals sets the sign bit, so the value comes out negative.decimal_test.ccalready had these cases commented out under "XXX conversion overflows are currently not detected".What changes are included in this PR?
ShiftAndAddnow returnsfalsewhen the carry out of the most significant word is non-zero. The multiply-add step is factored intoMultiplyAndAdd.MultiplyByPowerOfTen) before the sign is applied, so it is checked the same way. The existing-parsed_scale > kMaxScalecheck moves earlier. The error is the same as before.FitsInSignedWordsrejects magnitudes that don't fit the signed type.-2^(N-1)is still accepted, asDecimal128Test.PrintMinValuerequires.value > ValueType maxcheck. It now also runs on values that would have wrapped theuint64_taccumulator.All overflow cases return the existing error,
Invalid: The string '...' cannot be represented as decimalNNN.Are these changes tested?
Yes:
Decimal128Test.FromStringLimitsandDecimal256Test.FromStringLimitsare now enabled. I also added boundary checks: 2^127 - 1 and -2^127 parse, while 2^127, -2^127 - 1 and 2^128 fail. For 256-bit, 2^255 - 1 parses, while 2^255 and 2^256 + 7 fail.Decimal32Test.FromStringOverflowandDecimal64Test.FromStringOverflow.Cast.StringToDecimalOverflowwith the reproducer from the issue.The new tests fail without the change in
decimal.ccand pass with it. Locally on Linux (Ubuntu 26.04 under WSL, GCC, Boost 1.90):arrow-utility-test --gtest_filter=*Decimal*(327 tests) andarrow-compute-scalar-cast-test --gtest_filter=*Decimal*(26 tests) pass.Are there any user-facing changes?
Yes. Strings whose value does not fit in the decimal storage type now raise an error instead of returning a wrapped value.
This PR contains a "Critical Fix". Before this change, casting or parsing such strings silently produced incorrect data.
Was AI used for this PR?
In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.
PR code and description written by:
Reviewed before submission by: