Skip to content

GH-55606: [C++] Detect overflow when parsing decimals from strings - #55619

Open
csarushan1729 wants to merge 1 commit into
apache:mainfrom
csarushan1729:GH-55606
Open

csarushan1729 wants to merge 1 commit into
apache:mainfrom
csarushan1729:GH-55606

Conversation

@csarushan1729

@csarushan1729 csarushan1729 commented Nov 15, 2026 •

Copy link
Copy Markdown

Rationale for this change

Decimal{32,64,128,256}::FromString builds 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's FitsInPrecision() check then passes on the wrapped value. As a result, cast(string -> decimal) (and anything else built on FromString, such as the CSV reader) silently returns wrong values:

input target result before
"9" * 39 decimal128(38, 0) -20847100762815390390123822295304634369
2**64 decimal64(18, 0) 0
2**256 + 7 decimal256(76, 0) 7
2**128 - 1 decimal128(38, 0) -1
"99e38" decimal128(38, 0) 31811359292784559562136384478721867776
"9e9" decimal32(9, 0) 410065408

There are three ways this can wrap:

  1. Accumulating the digits (ShiftAndAdd drops the carry out of the top word).
  2. Scaling up by a positive exponent (*out *= GetScaleMultiplier(...) is unchecked).
  3. A magnitude in [2^(N-1), 2^N) for 128/256-bit decimals sets the sign bit, so the value comes out negative.

decimal_test.cc already had these cases commented out under "XXX conversion overflows are currently not detected".

What changes are included in this PR?

  • ShiftAndAdd now returns false when the carry out of the most significant word is non-zero. The multiply-add step is factored into MultiplyAndAdd.
  • The positive-exponent scale-up is done on the unsigned words (MultiplyByPowerOfTen) before the sign is applied, so it is checked the same way. The existing -parsed_scale > kMaxScale check moves earlier. The error is the same as before.
  • For Decimal128/256, FitsInSignedWords rejects magnitudes that don't fit the signed type. -2^(N-1) is still accepted, as Decimal128Test.PrintMinValue requires.
  • Decimal32/64 keep their existing value > ValueType max check. It now also runs on values that would have wrapped the uint64_t accumulator.

All overflow cases return the existing error, Invalid: The string '...' cannot be represented as decimalNNN.

Are these changes tested?

Yes:

  • The commented-out "XXX" overflow cases in Decimal128Test.FromStringLimits and Decimal256Test.FromStringLimits are 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.
  • New Decimal32Test.FromStringOverflow and Decimal64Test.FromStringOverflow.
  • New Cast.StringToDecimalOverflow with the reproducer from the issue.

The new tests fail without the change in decimal.cc and pass with it. Locally on Linux (Ubuntu 26.04 under WSL, GCC, Boost 1.90): arrow-utility-test --gtest_filter=*Decimal* (327 tests) and arrow-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:

  • Human
  • AI

Reviewed before submission by:

  • Human
  • AI
  • Not reviewed

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #55606 has been automatically assigned in GitHub to PR creator.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #55606 has no components, please add labels for components.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #55606 has no components, please add labels for components.

1 similar comment
@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #55606 has no components, please add labels for components.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant