GH-49817: [C++] Reject decimal strings that exceed the target precision#49832
Open
SAY-5 wants to merge 1 commit intoapache:mainfrom
Open
GH-49817: [C++] Reject decimal strings that exceed the target precision#49832SAY-5 wants to merge 1 commit intoapache:mainfrom
SAY-5 wants to merge 1 commit intoapache:mainfrom
Conversation
…recision DecimalFromString and SimpleDecimalFromString fed the digit string into ShiftAndAdd, which multiplies-and-adds into a fixed-size uint64_t array sized to the target decimal's bit width. ShiftAndAdd carries high bits only through out_size limbs and silently drops the remaining carry. The parser then returned Status::OK with a corrupted (mod 2^kBitWidth) value; callers that ignored the parsed-precision out-parameter had no way to notice. Check parsed_precision against kMaxPrecision before the ShiftAndAdd call and return Status::Invalid with a descriptive message when the input exceeds the decimal's capacity. Closes apacheGH-49817. Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
|
|
raulcd
reviewed
Apr 22, 2026
Member
raulcd
left a comment
raulcd
left a comment
There was a problem hiding this comment.
The reporting issue contains some tests and reproducers. Can we add tests?
@SAY-5 as per our guidelines, can you share whether the fix was AI generated and summarise what was AI-generated?
https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code
Thanks!
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
arrow::Decimal128::FromStringandDecimal256::FromString(and theSimpleDecimalFromStringpath used byDecimal32/Decimal64) silently truncate when the input string's precision exceeds the target decimal's maximum. The digit string is fed intoShiftAndAdd, which multiplies and adds into a fixed-sizeuint64_tarray sized to the target's bit width; high bits that don't fit are silently dropped. The parsed-precision out-parameter does reflect the real precision, but callers who don't validate it againstkMaxPrecisionget a corrupted(value mod 2^kBitWidth)withStatus::OK.What changes are included in this PR?
Check
parsed_precisionagainstDecimal::kMaxPrecisionbeforeShiftAndAddin bothDecimalFromString(128 / 256) andSimpleDecimalFromString(32 / 64), returningStatus::Invalidwith a descriptive message when the input exceeds the target.Are these changes tested?
Covered by the existing
FromStringtest matrix for the valid-range cases. Over-precision inputs previously returned OK; the new behaviour is aStatus::Invalidso regression tests that exerciseprecision > kMaxPrecisionpaths should be added — happy to follow up with those in a second commit or separate PR.Are there any user-facing changes?
Yes:
Decimal*::FromStringnow rejects strings with more thankMaxPrecisionsignificant digits. Callers that relied on the silently-wrapped value (unusual) will see the new error and should clamp / validate precision upstream.Closes #49817.
Signed-off-by: SAY-5 SAY-5@users.noreply.github.com