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.
Generated-by: GitHub Copilot CLI (GPT-5.6 Sol) Signed-off-by: Stefan Wang <1fannnw@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
Decimal32/Decimal64 parsing still incorrectly rejects the smallest negative in-range value due to a sign-insensitive magnitude check in the updated overflow-aware path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
Generated-by: GitHub Copilot CLI (GPT-5.6 Sol) Signed-off-by: Stefan Wang <1fannnw@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
Overflow rejection is currently conditional on out != nullptr, so callers requesting only precision/scale can still receive OK for unrepresentable inputs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
cpp/src/arrow/util/decimal.cc:986
- Same issue as above: representability/overflow checks are skipped entirely when
out == nullptr, so callers asking only forprecision/scalecould getOKfor values that wouldn't fit in the destination type.
if (out != nullptr) {
uint64_t value{0};
if (ShiftAndAddWithOverflow(dec.whole_digits, &value, 1, dec.sign == '-') ||
ShiftAndAddWithOverflow(dec.fractional_digits, &value, 1, dec.sign == '-') ||
value > static_cast<uint64_t>(
std::numeric_limits<typename DecimalClass::ValueType>::max()) +
static_cast<uint64_t>(dec.sign == '-')) {
return Status::Invalid("The string '", s, "' cannot be represented as ", type_name);
}
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
Generated-by: GitHub Copilot CLI (GPT-5.6 Sol) Signed-off-by: Stefan Wang <1fannnw@gmail.com>
There was a problem hiding this comment.
🟢 Approval recommended
The overflow detection is implemented in the shared accumulator path, is applied consistently across decimal widths, and is backed by focused boundary/overflow tests that cover the reported failure mode.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.