Copilot commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r4528769435
##########
cpp/src/gandiva/gdv_function_stubs.cc:
##########
@@ -204,11 +207,59 @@ 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;
+ int32_t input_scale = 0;
+ if (arrow::internal::ParseDecimalComponents(input.data(), input.size(),
+ &components) &&
+ !arrow::internal::SubtractWithOverflow(
+ static_cast<int32_t>(components.fractional_digits.size()),
+ components.exponent, &input_scale) &&
+ input_scale > out_scale) {
+ 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 - input_scale;
+ if (num_digits > arrow::Decimal128::kMaxPrecision) {
+ gdv_fn_context_set_error_msg(context, status.message().data());
Review Comment:
When `status.ok()` but the `(scale_from_str - out_scale)` check fails, the
`num_digits > kMaxPrecision` path sets the Gandiva error message from
`status.message()`, which will be empty for an OK status. This can surface as
an unhelpful/blank execution error; set an explicit message here (e.g., the
same “The string '…' cannot be represented as decimal128”) rather than relying
on `status.message()`.
##########
cpp/src/arrow/json/converter_test.cc:
##########
@@ -254,9 +254,15 @@ TEST(ConverterTest, Decimal128And256PrecisionError) {
std::shared_ptr<StructArray> parse_array;
ASSERT_OK(ParseFromString(options, json_source, &parse_array));
- std::string error_msg =
- "Invalid: Failed to convert JSON to " + decimal_type->ToString() +
- ": 123456789012345678901234567890.0123456789 requires precision 40";
+ std::string error_msg;
+ if (decimal_type->id() == Type::DECIMAL128) {
+ error_msg =
+ "Invalid: The string '123456789012345678901234567890.0123456789' "
+ "cannot be represented as decimal128";
+ } else {
+ error_msg = "Invalid: Failed to convert JSON to " +
decimal_type->ToString() +
+ ": 123456789012345678901234567890.0123456789 requires
precision 40";
+ }
Review Comment:
The Decimal128 branch hard-codes an error string that may not be a substring
of the actual JSON converter error if the converter wraps the underlying parse
failure (e.g., prefixing with “Failed to convert JSON to …:”). To make the test
robust, assert on a stable substring for Decimal128 (such as “cannot be
represented as decimal128” or “The string '…' cannot be represented as
decimal128”) rather than including the leading “Invalid: ” and omitting any
converter prefix.
##########
cpp/src/gandiva/gdv_function_stubs.cc:
##########
@@ -204,11 +207,59 @@ 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;
+ int32_t input_scale = 0;
+ if (arrow::internal::ParseDecimalComponents(input.data(), input.size(),
+ &components) &&
+ !arrow::internal::SubtractWithOverflow(
+ static_cast<int32_t>(components.fractional_digits.size()),
+ components.exponent, &input_scale) &&
+ input_scale > out_scale) {
+ 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 - input_scale;
+ 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";
+ }
+ }
Review Comment:
This fallback path copies the full whole+fractional digit sequence into a
new `std::string`. For very large inputs, this can create significant extra
allocation and CPU overhead (and may be attacker-controlled in some
deployments). Consider avoiding a full copy by computing `num_digits` and the
rounding digit using the original `string_view`s, and only materializing at
most the prefix needed to parse (bounded by `kMaxPrecision + 1` digits).
--
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]
