GH-49817: [C++] Reject overflowing decimal strings - #51169
Conversation
Generated-by: GitHub Copilot CLI (GPT-5.6 Sol) Signed-off-by: 1fanwang <1fannnw@gmail.com>
|
|
There was a problem hiding this comment.
🟡 Changes recommended
Decimal32/Decimal64 parsing still incorrectly rejects the minimum negative representable value due to a value > max() magnitude check that doesn’t allow -2^(N-1).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses integer overflow during C++ decimal string parsing so that oversized inputs no longer wrap modulo the destination bit width while still returning Status::OK(). It does so by adding overflow detection to the digit-accumulation path and rejecting magnitudes outside the signed integer range before constructing Decimal128/Decimal256 values.
Changes:
- Add carry/overflow reporting to the digit accumulator used by
FromStringso overflow is detected instead of silently dropped. - Reject parsed magnitudes outside the signed representable range (e.g., > 2^(N-1)-1, or < -2^(N-1)) for wide decimals prior to constructing the decimal value.
- Extend
Decimal128Test/Decimal256Testlimit coverage with overflow-focused cases matching the reported issue.
File summaries
| File | Description |
|---|---|
| cpp/src/arrow/util/decimal.cc | Introduces overflow-aware accumulation and signed-range magnitude rejection during decimal parsing. |
| cpp/src/arrow/util/decimal_test.cc | Adds regression tests ensuring oversized Decimal128/Decimal256 strings now return Invalid. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (ShiftAndAddWithOverflow(dec.whole_digits, &value, 1) || | ||
| ShiftAndAddWithOverflow(dec.fractional_digits, &value, 1) || | ||
| value > static_cast<uint64_t>( | ||
| std::numeric_limits<typename DecimalClass::ValueType>::max())) { | ||
| return Status::Invalid("The string '", s, "' cannot be represented as ", type_name); |
Rationale for this change
Parsing an oversized decimal string can return
OKwith a wrapped value instead of rejecting the input. For the 51-digit input in #49817, the returned integer is the input modulo 2^128 instead of the parsed value.What changes are included in this PR?
The digit accumulator now reports carry beyond the destination limbs. The parser also rejects magnitudes outside the signed range before constructing the decimal value. Decimal32 and Decimal64 use the same carry check.
This leaves precision, scale, and Gandiva rounding policy unchanged. It only prevents integer wrap from being reported as a successful parse.
Closes #49817
Are these changes tested?
OKwith wrapped dataInvalidOKwith wrapped dataInvalidFromStringcoverageRaw logs
Are there any user-facing changes?
Yes. Decimal strings that exceed the target integer range now return
Invalidinstead of a corrupted value.This PR contains a "Critical Fix". It prevents the decimal parser from returning incorrect data after integer overflow.