Copilot commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r4067952803
##########
cpp/src/arrow/util/decimal.cc:
##########
@@ -892,12 +904,15 @@ Status DecimalFromString(const char* type_name,
std::string_view s, Decimal* out
parsed_scale = static_cast<int32_t>(dec.fractional_digits.size());
}
+ static_assert(Decimal::kBitWidth % 64 == 0, "decimal bit-width not a
multiple of 64");
+ std::array<uint64_t, Decimal::kBitWidth / 64> little_endian_array{};
+ if (ShiftAndAddWithOverflow(dec.whole_digits, little_endian_array.data(),
+ little_endian_array.size(), dec.sign == '-') ||
+ ShiftAndAddWithOverflow(dec.fractional_digits,
little_endian_array.data(),
+ little_endian_array.size(), dec.sign == '-')) {
+ return Status::Invalid("The string '", s, "' cannot be represented as ",
type_name);
Review Comment:
This only checks the digit limbs before the positive-exponent scale
increase. For example, `Decimal128::FromString("99e38")` passes these checks,
then `*out *= GetScaleMultiplier(38)` truncates the 9.9e39 product because
BasicDecimal multiplication is wrapping; the status is still OK. The same gap
exists in `SimpleDecimalFromString` (`99e9`/`99e18`), and metadata-only calls
skip the multiplication entirely, so validate the scale increase with overflow
detection in both paths and add regressions for `out == nullptr`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
