Copilot commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r4091549808
##########
cpp/src/gandiva/gdv_function_stubs.cc:
##########
@@ -204,11 +206,55 @@ CRC_FUNCTION(utf8)
CRC_FUNCTION(binary)
int32_t gdv_fn_dec_from_string(int64_t context, const char* in, int32_t
in_length,
- int32_t* precision_from_str, int32_t*
scale_from_str,
- int64_t* dec_high_from_str, uint64_t*
dec_low_from_str) {
+ int32_t out_scale, int32_t* precision_from_str,
+ int32_t* scale_from_str, int64_t*
dec_high_from_str,
+ uint64_t* dec_low_from_str) {
arrow::Decimal128 dec;
- auto status = arrow::Decimal128::FromString(std::string(in, in_length), &dec,
- precision_from_str,
scale_from_str);
+ const std::string_view input(in, in_length);
+ auto status =
+ arrow::Decimal128::FromString(input, &dec, precision_from_str,
scale_from_str);
+ if (!status.ok() ||
+ static_cast<int64_t>(*scale_from_str) - out_scale >
arrow::Decimal128::kMaxScale) {
+ arrow::internal::DecimalComponents components;
+ if (arrow::internal::ParseDecimalComponents(input.data(), input.size(),
+ &components)) {
+ std::string digits(components.whole_digits);
+ digits.append(components.fractional_digits);
+ digits.erase(0, digits.find_first_not_of('0'));
+ const int64_t num_digits =
+ static_cast<int64_t>(digits.size()) + out_scale -
+ static_cast<int64_t>(components.fractional_digits.size()) +
components.exponent;
+ if (num_digits > arrow::Decimal128::kMaxPrecision) {
+ gdv_fn_context_set_error_msg(context, status.message().data());
+ return -1;
+ }
+ bool round_up = false;
+ if (num_digits < 0) {
+ digits = "0";
+ } else {
+ round_up = num_digits < static_cast<int64_t>(digits.size()) &&
+ digits[static_cast<size_t>(num_digits)] >= '5';
+ digits.resize(static_cast<size_t>(num_digits), '0');
+ if (digits.empty()) {
+ digits = "0";
+ }
+ }
+ status =
+ arrow::Decimal128::FromString(digits, &dec, precision_from_str,
scale_from_str);
+ if (status.ok()) {
+ if (round_up) {
+ dec += arrow::Decimal128(1);
+ if (dec ==
arrow::Decimal128::GetScaleMultiplier(*precision_from_str)) {
+ ++(*precision_from_str);
+ }
+ }
+ if (components.sign == '-') {
+ dec.Negate();
+ }
+ *scale_from_str = out_scale;
Review Comment:
After the HALF_UP increment, this path can produce a value whose precision
is greater than `out_precision` (for example, `"99." + 80 nines` cast to
DECIMAL(4,2) becomes 10000 and increments `precision_from_str` to 5). Assigning
`scale_from_str = out_scale` then makes `decimalops::Convert` take its
same-scale fast path and skip the precision check, so the test's expected 0.00
is not produced and an out-of-range value is emitted. Validate the rounded
result against `out_precision` before normalizing the scale, or preserve a
conversion path that performs the overflow check.
--
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]
