You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "1fanwang (via GitHub)" <gi...@apache.org> on 2026/09/06 15:00:33 UTC
[PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
1fanwang opened a new pull request, #51169:
URL: https://github.com/apache/arrow/pull/51169
### Rationale for this change
Parsing an oversized decimal string can return `OK` with a wrapped value instead of rejecting the input. For the 51-digit input in https://github.com/apache/arrow/issues/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 https://github.com/apache/arrow/issues/49817
### Are these changes tested?
| Scenario | Before | After |
|---|---|---|
| Oversized Decimal128 input | Returns `OK` with wrapped data | Returns `Invalid` |
| Oversized Decimal256 input | Returns `OK` with wrapped data | Returns `Invalid` |
| Existing `FromString` coverage | Not applicable | Passes |
<details>
<summary>Raw logs</summary>
```text
$ cpp/build/debug/arrow-utility-test \
--gtest_filter='Decimal128Test.FromStringLimits:Decimal256Test.FromStringLimits'
Before:
Expected Decimal128::FromString(...) to fail with Invalid, but got OK
Expected Decimal256::FromString(...) to fail with Invalid, but got OK
2 FAILED TESTS
$ cpp/build/debug/arrow-utility-test --gtest_filter='*FromString*'
After:
[==========] Running 48 tests from 7 test suites.
[ PASSED ] 48 tests.
```
</details>
### Are there any user-facing changes?
Yes. Decimal strings that exceed the target integer range now return `Invalid` instead of a corrupted value.
**This PR contains a "Critical Fix".** It prevents the decimal parser from returning incorrect data after integer overflow.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51169:
URL: https://github.com/apache/arrow/pull/51169#issuecomment-5809221394
:warning: GitHub issue #49817 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "1fanwang (via GitHub)" <gi...@apache.org>.
1fanwang commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r3952490925
##########
cpp/src/arrow/util/decimal.cc:
##########
@@ -783,8 +784,25 @@ static inline void ShiftAndAdd(std::string_view input, uint64_t out[], size_t ou
out[i] = static_cast<uint64_t>(tmp & 0xFFFFFFFFFFFFFFFFULL);
chunk = static_cast<uint64_t>(tmp >> 64);
}
+ if (chunk != 0) {
Review Comment:
Done in https://github.com/apache/arrow/commit/0d5bf6d4bc2ffb22ad4f37783fdb6bb174da8e70.
##########
cpp/src/arrow/util/decimal.cc:
##########
@@ -783,8 +784,25 @@ static inline void ShiftAndAdd(std::string_view input, uint64_t out[], size_t ou
out[i] = static_cast<uint64_t>(tmp & 0xFFFFFFFFFFFFFFFFULL);
chunk = static_cast<uint64_t>(tmp >> 64);
}
+ if (chunk != 0) {
+ return true;
+ }
posn += group_size;
}
+ return false;
+}
+
+static inline bool MagnitudeOverflowsSignedDecimal(const uint64_t out[], size_t out_size,
Review Comment:
Done in https://github.com/apache/arrow/commit/0d5bf6d4bc2ffb22ad4f37783fdb6bb174da8e70.
##########
cpp/src/arrow/util/decimal_test.cc:
##########
@@ -550,6 +555,9 @@ TEST(Decimal256Test, FromStringLimits) {
// Decimal256::FromString("-9999999999999999999999999999999999999999999999999999999999999999999999999999e1"));
// ASSERT_RAISES(Invalid,
// Decimal256::FromString("99999999999999999999999999999999999999999999999999999999999999999999999999999"));
+ ASSERT_RAISES(Invalid, Decimal256::FromString(std::string(78, '9')));
+ ASSERT_RAISES(Invalid, Decimal256::FromString("5789604461865809771178549250434395392663"
+ "4992332820282019728792003956564819968"));
Review Comment:
Done in https://github.com/apache/arrow/commit/0d5bf6d4bc2ffb22ad4f37783fdb6bb174da8e70.
##########
cpp/src/arrow/util/decimal.cc:
##########
@@ -962,9 +977,9 @@ Status SimpleDecimalFromString(const char* type_name, std::string_view s,
if (out != nullptr) {
uint64_t value{0};
- ShiftAndAdd(dec.whole_digits, &value, 1);
- ShiftAndAdd(dec.fractional_digits, &value, 1);
- if (value > static_cast<uint64_t>(
+ 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())) {
Review Comment:
Done in https://github.com/apache/arrow/commit/aa6fc1e361b3c618e8f2ed275ee9ad47b341ad19.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51169:
URL: https://github.com/apache/arrow/pull/51169#issuecomment-5560078948
:warning: GitHub issue #49817 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "1fanwang (via GitHub)" <gi...@apache.org>.
1fanwang commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r4076584617
##########
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:
Done in https://github.com/apache/arrow/commit/1d3bdbec660314ea42800d1d5e477a6cea98100d.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "1fanwang (via GitHub)" <gi...@apache.org>.
1fanwang commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r4090725100
##########
cpp/src/arrow/util/decimal.cc:
##########
@@ -892,16 +904,17 @@ Status DecimalFromString(const char* type_name, std::string_view s, Decimal* out
parsed_scale = static_cast<int32_t>(dec.fractional_digits.size());
}
- if (out != nullptr) {
- static_assert(Decimal::kBitWidth % 64 == 0, "decimal bit-width not a multiple of 64");
- std::array<uint64_t, Decimal::kBitWidth / 64> little_endian_array{};
- ShiftAndAdd(dec.whole_digits, little_endian_array.data(), little_endian_array.size());
- ShiftAndAdd(dec.fractional_digits, little_endian_array.data(),
- little_endian_array.size());
- *out = Decimal(bit_util::little_endian::ToNative(little_endian_array));
- if (dec.sign == '-') {
- out->Negate();
- }
+ 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:
Done in https://github.com/apache/arrow/commit/cccb72a1f548a961fa1786acac1c627145a1d599.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r3948274157
##########
cpp/src/arrow/util/decimal.cc:
##########
@@ -783,8 +784,25 @@ static inline void ShiftAndAdd(std::string_view input, uint64_t out[], size_t ou
out[i] = static_cast<uint64_t>(tmp & 0xFFFFFFFFFFFFFFFFULL);
chunk = static_cast<uint64_t>(tmp >> 64);
}
+ if (chunk != 0) {
Review Comment:
Should we also check `out[size - 1] & kSignBit` to detect overflow into the sign bit here?
##########
cpp/src/arrow/util/decimal_test.cc:
##########
@@ -550,6 +555,9 @@ TEST(Decimal256Test, FromStringLimits) {
// Decimal256::FromString("-9999999999999999999999999999999999999999999999999999999999999999999999999999e1"));
// ASSERT_RAISES(Invalid,
// Decimal256::FromString("99999999999999999999999999999999999999999999999999999999999999999999999999999"));
+ ASSERT_RAISES(Invalid, Decimal256::FromString(std::string(78, '9')));
+ ASSERT_RAISES(Invalid, Decimal256::FromString("5789604461865809771178549250434395392663"
+ "4992332820282019728792003956564819968"));
Review Comment:
Also check negative overflow?
##########
cpp/src/arrow/util/decimal.cc:
##########
@@ -783,8 +784,25 @@ static inline void ShiftAndAdd(std::string_view input, uint64_t out[], size_t ou
out[i] = static_cast<uint64_t>(tmp & 0xFFFFFFFFFFFFFFFFULL);
chunk = static_cast<uint64_t>(tmp >> 64);
}
+ if (chunk != 0) {
+ return true;
+ }
posn += group_size;
}
+ return false;
+}
+
+static inline bool MagnitudeOverflowsSignedDecimal(const uint64_t out[], size_t out_size,
Review Comment:
I'm not sure we need a separate function since the check can probably be done in `ShiftAndAddWithOverflow` (see previous comment above).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51169:
URL: https://github.com/apache/arrow/pull/51169#issuecomment-5770197973
:warning: GitHub issue #49817 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "1fanwang (via GitHub)" <gi...@apache.org>.
1fanwang commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r4091628988
##########
cpp/src/gandiva/gdv_function_stubs.cc:
##########
@@ -204,11 +206,51 @@ 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;
+ bool round_up = false;
+ if (num_digits < 0 || num_digits > arrow::Decimal128::kMaxPrecision) {
+ digits = "0";
Review Comment:
Done in https://github.com/apache/arrow/commit/240f1d13baf074e410ef1b605a484cc6cac7cfca; invalid scales and oversized coefficients remain errors.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "1fanwang (via GitHub)" <gi...@apache.org>.
1fanwang commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r4091188155
##########
cpp/src/gandiva/gdv_function_stubs.cc:
##########
@@ -204,11 +206,51 @@ 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;
+ bool round_up = false;
+ if (num_digits < 0 || num_digits > arrow::Decimal128::kMaxPrecision) {
+ digits = "0";
Review Comment:
Done in https://github.com/apache/arrow/commit/faa9b4fe6ee44f1fd41ff00e875c13d499dc21ff.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "1fanwang (via GitHub)" <gi...@apache.org>.
1fanwang commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r4091513899
##########
cpp/src/gandiva/gdv_function_stubs.cc:
##########
@@ -204,11 +206,51 @@ 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;
+ bool round_up = false;
+ if (num_digits < 0 || num_digits > arrow::Decimal128::kMaxPrecision) {
+ digits = "0";
Review Comment:
Done in https://github.com/apache/arrow/commit/1f4c212b420ad43965df65e9be0ce6d4a48fbbee; tiny values still round to zero, as 1e-30 does.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51169:
URL: https://github.com/apache/arrow/pull/51169#issuecomment-5810674404
:warning: GitHub issue #49817 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "1fanwang (via GitHub)" <gi...@apache.org>.
1fanwang commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r4091703068
##########
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:
`Convert` still checks precision at the same scale, and this input returns 0.00 through the projector.
##########
cpp/src/gandiva/gdv_function_stubs.cc:
##########
@@ -204,11 +206,51 @@ 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;
+ bool round_up = false;
+ if (num_digits < 0 || num_digits > arrow::Decimal128::kMaxPrecision) {
+ digits = "0";
Review Comment:
Done in https://github.com/apache/arrow/commit/1f4c212b420ad43965df65e9be0ce6d4a48fbbee.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "1fanwang (via GitHub)" <gi...@apache.org>.
1fanwang commented on PR #51169:
URL: https://github.com/apache/arrow/pull/51169#issuecomment-6313707160
Could a maintainer rerun [ARM macOS C++](https://github.com/apache/arrow/actions/runs/39579254721/job/119152474046) and [Intel macOS Python](https://github.com/apache/arrow/actions/runs/39579254421/job/119152472360)? They hit S3/MinIO timeouts and a CSV latency assertion; Gandiva passed.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51169:
URL: https://github.com/apache/arrow/pull/51169#issuecomment-5839550468
:warning: GitHub issue #49817 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51169:
URL: https://github.com/apache/arrow/pull/51169#issuecomment-5783643370
:warning: GitHub issue #49817 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r4076240046
##########
cpp/src/arrow/util/decimal.cc:
##########
@@ -892,16 +904,17 @@ Status DecimalFromString(const char* type_name, std::string_view s, Decimal* out
parsed_scale = static_cast<int32_t>(dec.fractional_digits.size());
}
- if (out != nullptr) {
- static_assert(Decimal::kBitWidth % 64 == 0, "decimal bit-width not a multiple of 64");
- std::array<uint64_t, Decimal::kBitWidth / 64> little_endian_array{};
- ShiftAndAdd(dec.whole_digits, little_endian_array.data(), little_endian_array.size());
- ShiftAndAdd(dec.fractional_digits, little_endian_array.data(),
- little_endian_array.size());
- *out = Decimal(bit_util::little_endian::ToNative(little_endian_array));
- if (dec.sign == '-') {
- out->Negate();
- }
+ 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 range check rejects the complete unscaled coefficient before callers can apply a target-aware scale conversion. In particular, Gandiva's `castDECIMAL_utf8` calls `Decimal128::FromString` through `gdv_fn_dec_from_string` and only then calls `decimalops::Convert`; inputs such as `1.55555555555555555555555555555555555555555555555555` for `DECIMAL(38,37)` now fail and produce the cast error/zero instead of the documented HALF_UP result. Please add the caller-side rounding/normalization (or another target-aware parsing path) before `FromString`, with an end-to-end regression test.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51169:
URL: https://github.com/apache/arrow/pull/51169#issuecomment-6311982506
:warning: GitHub issue #49817 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51169:
URL: https://github.com/apache/arrow/pull/51169#issuecomment-5810864363
:warning: GitHub issue #49817 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r3944360018
##########
cpp/src/arrow/util/decimal.cc:
##########
@@ -962,9 +985,9 @@ Status SimpleDecimalFromString(const char* type_name, std::string_view s,
if (out != nullptr) {
uint64_t value{0};
- ShiftAndAdd(dec.whole_digits, &value, 1);
- ShiftAndAdd(dec.fractional_digits, &value, 1);
- if (value > static_cast<uint64_t>(
+ 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);
Review Comment:
SimpleDecimalFromString rejects the minimum negative value for Decimal32/Decimal64. For example, parsing "-2147483648" / "-9223372036854775808" should be representable in two’s complement, but the current check `value > max()` rejects it before applying the sign.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r3952503516
##########
cpp/src/arrow/util/decimal.cc:
##########
@@ -895,9 +907,12 @@ Status DecimalFromString(const char* type_name, std::string_view s, Decimal* out
if (out != nullptr) {
static_assert(Decimal::kBitWidth % 64 == 0, "decimal bit-width not a multiple of 64");
std::array<uint64_t, Decimal::kBitWidth / 64> little_endian_array{};
- ShiftAndAdd(dec.whole_digits, little_endian_array.data(), little_endian_array.size());
- ShiftAndAdd(dec.fractional_digits, little_endian_array.data(),
- little_endian_array.size());
+ 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:
Overflow validation is only performed when `out != nullptr`. Calling `FromString` with `out == nullptr` (but requesting `precision`/`scale`) would still return `OK` for values that cannot be represented in the target bit width, which is inconsistent with the new overflow-rejection behavior.
This issue also appears on line 978 of the same file.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "1fanwang (via GitHub)" <gi...@apache.org>.
1fanwang commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r3953484970
##########
cpp/src/arrow/util/decimal.cc:
##########
@@ -895,9 +907,12 @@ Status DecimalFromString(const char* type_name, std::string_view s, Decimal* out
if (out != nullptr) {
static_assert(Decimal::kBitWidth % 64 == 0, "decimal bit-width not a multiple of 64");
std::array<uint64_t, Decimal::kBitWidth / 64> little_endian_array{};
- ShiftAndAdd(dec.whole_digits, little_endian_array.data(), little_endian_array.size());
- ShiftAndAdd(dec.fractional_digits, little_endian_array.data(),
- little_endian_array.size());
+ 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:
Done in https://github.com/apache/arrow/commit/4e1279bfd6336701ff9f1ffc23fe96c5d9bccd45.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51169:
URL: https://github.com/apache/arrow/pull/51169#issuecomment-5810078058
:warning: GitHub issue #49817 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r4090753494
##########
cpp/src/gandiva/gdv_function_stubs.cc:
##########
@@ -204,11 +206,51 @@ 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;
+ bool round_up = false;
+ if (num_digits < 0 || num_digits > arrow::Decimal128::kMaxPrecision) {
+ digits = "0";
Review Comment:
This fallback runs for every non-OK result from `Decimal128::FromString`, not only raw-limb overflow. A validly parsed but out-of-range exponent is therefore converted to zero: for example, `1e2147483647` is rejected by the parser because its required positive scale exceeds `kMaxScale`, but `num_digits > kMaxPrecision` sets `digits` to `"0"`, the retry succeeds, and the cast returns zero. The same issue occurs for exponent/scale arithmetic overflow such as `1e-2147483648`; distinguish recoverable rescaling cases from parser range errors before retrying.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r3952477332
##########
cpp/src/arrow/util/decimal.cc:
##########
@@ -962,9 +977,9 @@ Status SimpleDecimalFromString(const char* type_name, std::string_view s,
if (out != nullptr) {
uint64_t value{0};
- ShiftAndAdd(dec.whole_digits, &value, 1);
- ShiftAndAdd(dec.fractional_digits, &value, 1);
- if (value > static_cast<uint64_t>(
+ 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())) {
Review Comment:
`SimpleDecimalFromString` still rejects the smallest negative value for Decimal32/Decimal64 (e.g. -2^63) because it compares the parsed magnitude against `numeric_limits<...>::max()` regardless of sign. This is inconsistent with `ShiftAndAddWithOverflow(..., negative=true)`, which explicitly allows the `kSignBit` magnitude needed to represent INT_MIN/INT64_MIN, and it prevents parsing valid in-range values.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "1fanwang (via GitHub)" <gi...@apache.org>.
1fanwang commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r3953564408
##########
cpp/src/arrow/util/decimal.cc:
##########
@@ -962,9 +985,9 @@ Status SimpleDecimalFromString(const char* type_name, std::string_view s,
if (out != nullptr) {
uint64_t value{0};
- ShiftAndAdd(dec.whole_digits, &value, 1);
- ShiftAndAdd(dec.fractional_digits, &value, 1);
- if (value > static_cast<uint64_t>(
+ 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);
Review Comment:
Done in [aa6fc1e](https://github.com/apache/arrow/commit/aa6fc1e361b3c618e8f2ed275ee9ad47b341ad19).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51169:
URL: https://github.com/apache/arrow/pull/51169#issuecomment-5810724269
:warning: GitHub issue #49817 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51169:
URL: https://github.com/apache/arrow/pull/51169#issuecomment-5809610088
:warning: GitHub issue #49817 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51169:
URL: https://github.com/apache/arrow/pull/51169#issuecomment-5839457216
:warning: GitHub issue #49817 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51169:
URL: https://github.com/apache/arrow/pull/51169#issuecomment-6312910983
:warning: GitHub issue #49817 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r4334935090
##########
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'));
Review Comment:
When the input is an exactly-zero value with more fractional digits than `out_scale` (for example, `0.` followed by 100 zeros cast to DECIMAL(38,0)), `find_first_not_of` returns `npos`. Calling `erase(0, npos)` throws `std::out_of_range`, so the Gandiva projector can abort instead of returning zero. Handle the all-zero case explicitly before erasing leading zeros.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49817: [C++] Reject overflowing decimal strings [arrow]
Posted by "1fanwang (via GitHub)" <gi...@apache.org>.
1fanwang commented on code in PR #51169:
URL: https://github.com/apache/arrow/pull/51169#discussion_r4335345331
##########
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'));
Review Comment:
erase(0, npos) clears the string. The cast returns zero; I added [coverage](https://github.com/apache/arrow/commit/be36e4fcac4b344c6ebdf2ad2558e16ab514dcd9).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org