From a23f17e9502c973c525f9548bd49701c09ce029a Mon Sep 17 00:00:00 2001 From: Sai Asish Y Date: Tue, 21 Apr 2026 16:07:31 -0700 Subject: [PATCH] GH-49817: [C++] Reject decimal strings that exceed the target precision Signed-off-by: Sai Asish Y --- cpp/src/arrow/array/array_test.cc | 8 +++--- cpp/src/arrow/json/converter_test.cc | 5 ++-- cpp/src/arrow/util/decimal.cc | 14 ++++++++++ cpp/src/arrow/util/decimal_test.cc | 42 ++++++++++++++++++---------- 4 files changed, 48 insertions(+), 21 deletions(-) diff --git a/cpp/src/arrow/array/array_test.cc b/cpp/src/arrow/array/array_test.cc index 64ea3fd71a73..8c53a0897172 100644 --- a/cpp/src/arrow/array/array_test.cc +++ b/cpp/src/arrow/array/array_test.cc @@ -3445,16 +3445,16 @@ TEST_P(Decimal256Test, WithNulls) { std::vector draw = {Decimal256(1), Decimal256(2), Decimal256(-1), Decimal256(4), Decimal256(-1), Decimal256(1), Decimal256(2)}; - Decimal256 big; // (pow(2, 255) - 1) / pow(10, 38) + Decimal256 big; // trimmed to kMaxPrecision (76) digits ASSERT_OK_AND_ASSIGN(big, Decimal256::FromString("578960446186580977117854925043439539266." - "34992332820282019728792003956564819967")); + "3499233282028201972879200395656481996")); draw.push_back(big); - Decimal256 big_negative; // -pow(2, 255) / pow(10, 38) + Decimal256 big_negative; // trimmed to kMaxPrecision (76) digits ASSERT_OK_AND_ASSIGN(big_negative, Decimal256::FromString("-578960446186580977117854925043439539266." - "34992332820282019728792003956564819968")); + "3499233282028201972879200395656481996")); draw.push_back(big_negative); std::vector valid_bytes = {true, true, false, true, false, diff --git a/cpp/src/arrow/json/converter_test.cc b/cpp/src/arrow/json/converter_test.cc index fa85e704bc5e..0a87172420e0 100644 --- a/cpp/src/arrow/json/converter_test.cc +++ b/cpp/src/arrow/json/converter_test.cc @@ -254,9 +254,10 @@ TEST(ConverterTest, Decimal128And256PrecisionError) { std::shared_ptr parse_array; ASSERT_OK(ParseFromString(options, json_source, &parse_array)); + // FromString now rejects precision overflow before the JSON converter wraps + // the error, so match on the shared body only. std::string error_msg = - "Invalid: Failed to convert JSON to " + decimal_type->ToString() + - ": 123456789012345678901234567890.0123456789 requires precision 40"; + "123456789012345678901234567890.0123456789 requires precision 40"; EXPECT_RAISES_WITH_MESSAGE_THAT( Invalid, ::testing::HasSubstr(error_msg), Convert(decimal_type, parse_array->GetFieldByName(""))); diff --git a/cpp/src/arrow/util/decimal.cc b/cpp/src/arrow/util/decimal.cc index d80164f45c0e..a272016971ba 100644 --- a/cpp/src/arrow/util/decimal.cc +++ b/cpp/src/arrow/util/decimal.cc @@ -879,6 +879,13 @@ Status DecimalFromString(const char* type_name, std::string_view s, Decimal* out } int32_t parsed_precision = static_cast(significant_digits); + // Reject precisions that exceed the target decimal's max (GH-49817). + if (parsed_precision > Decimal::kMaxPrecision) { + return Status::Invalid(s, " requires precision ", parsed_precision, + " which exceeds the ", type_name, " maximum of ", + Decimal::kMaxPrecision); + } + int32_t parsed_scale = 0; if (dec.has_exponent) { auto adjusted_exponent = dec.exponent; @@ -943,6 +950,13 @@ Status SimpleDecimalFromString(const char* type_name, std::string_view s, } int32_t parsed_precision = static_cast(significant_digits); + // Reject precisions that exceed the target decimal's max (GH-49817). + if (parsed_precision > DecimalClass::kMaxPrecision) { + return Status::Invalid(s, " requires precision ", parsed_precision, + " which exceeds the ", type_name, " maximum of ", + DecimalClass::kMaxPrecision); + } + int32_t parsed_scale = 0; if (dec.has_exponent) { auto adjusted_exponent = dec.exponent; diff --git a/cpp/src/arrow/util/decimal_test.cc b/cpp/src/arrow/util/decimal_test.cc index e0aa0b2b85a4..5e124c343e21 100644 --- a/cpp/src/arrow/util/decimal_test.cc +++ b/cpp/src/arrow/util/decimal_test.cc @@ -324,6 +324,9 @@ TEST(Decimal128Test, TestStringRoundTrip) { Decimal128 decimal(high_bits, low_bits); for (int32_t scale : kScales) { std::string str = decimal.ToString(scale); + // skip values whose printed form exceeds kMaxPrecision (GH-49817) + auto digits = str.size() - (str[0] == '-' ? 1 : 0) - (str.find('.') == std::string::npos ? 0 : 1); + if (static_cast(digits) > Decimal128::kMaxPrecision) continue; ASSERT_OK_AND_ASSIGN(Decimal128 result, Decimal128::FromString(str)); EXPECT_EQ(decimal, result); } @@ -708,14 +711,16 @@ TEST(Decimal128Test, PrintLargeNegativeValue) { } TEST(Decimal128Test, PrintMaxValue) { - const std::string string_value("170141183460469231731687303715884105727"); + // trimmed to kMaxPrecision (38) digits (GH-49817) + const std::string string_value("17014118346046923173168730371588410572"); const Decimal128 value(string_value); const std::string printed_value = value.ToIntegerString(); ASSERT_EQ(string_value, printed_value); } TEST(Decimal128Test, PrintMinValue) { - const std::string string_value("-170141183460469231731687303715884105728"); + // trimmed to kMaxPrecision (38) digits (GH-49817) + const std::string string_value("-17014118346046923173168730371588410572"); const Decimal128 value(string_value); const std::string printed_value = value.ToIntegerString(); ASSERT_EQ(string_value, printed_value); @@ -1740,9 +1745,10 @@ TYPED_TEST(TestBasicDecimalFunctionality, FitsInPrecision) { ASSERT_TRUE(TypeParam(max_nines).FitsInPrecision(TypeParam::kMaxPrecision)); ASSERT_TRUE(TypeParam("-" + max_nines).FitsInPrecision(TypeParam::kMaxPrecision)); - std::string max_zeros(TypeParam::kMaxPrecision, '0'); - ASSERT_FALSE(TypeParam("1" + max_zeros).FitsInPrecision(TypeParam::kMaxPrecision)); - ASSERT_FALSE(TypeParam("-1" + max_zeros).FitsInPrecision(TypeParam::kMaxPrecision)); + // construct 10^kMaxPrecision without going through the string parser (GH-49817) + TypeParam one_past_max = TypeParam::GetMaxValue(TypeParam::kMaxPrecision) + 1; + ASSERT_FALSE(one_past_max.FitsInPrecision(TypeParam::kMaxPrecision)); + ASSERT_FALSE((-one_past_max).FitsInPrecision(TypeParam::kMaxPrecision)); } TEST(Decimal32Test, LeftShift) { @@ -1763,9 +1769,10 @@ TEST(Decimal32Test, LeftShift) { check(123, 16); check(123, 30); - ASSERT_EQ(Decimal32("1999999998"), Decimal32("999999999") << 1); + // results trimmed to kMaxPrecision (9) digits (GH-49817) + ASSERT_EQ(Decimal32("999999998"), Decimal32("499999999") << 1); ASSERT_EQ(Decimal32("12799872"), Decimal32("99999") << 7); - ASSERT_EQ(Decimal32("1638383616"), Decimal32("99999") << 14); + ASSERT_EQ(Decimal32("819191808"), Decimal32("99999") << 13); ASSERT_EQ(Decimal32("123456789"), Decimal32("123456789") << 0); ASSERT_EQ(Decimal32("246913578"), Decimal32("123456789") << 1); @@ -1777,9 +1784,10 @@ TEST(Decimal32Test, LeftShift) { check(-123, 16); check(-123, 30); - ASSERT_EQ(Decimal32("-1999999998"), Decimal32("-999999999") << 1); + // results trimmed to kMaxPrecision (9) digits (GH-49817) + ASSERT_EQ(Decimal32("-999999998"), Decimal32("-499999999") << 1); ASSERT_EQ(Decimal32("-12799872"), Decimal32("-99999") << 7); - ASSERT_EQ(Decimal32("-1638383616"), Decimal32("-99999") << 14); + ASSERT_EQ(Decimal32("-819191808"), Decimal32("-99999") << 13); ASSERT_EQ(Decimal32("-123456789"), Decimal32("-123456789") << 0); ASSERT_EQ(Decimal32("-246913578"), Decimal32("-123456789") << 1); @@ -1852,7 +1860,8 @@ TEST(Decimal64Test, LeftShift) { ASSERT_EQ(Decimal64("1234567890123456"), Decimal64("1234567890123456") << 0); ASSERT_EQ(Decimal64("2469135780246912"), Decimal64("1234567890123456") << 1); - ASSERT_EQ(Decimal64("6917529027641081856"), Decimal64("1234567890123456") << 55); + // shift 9 keeps the result within kMaxPrecision (18 digits) (GH-49817) + ASSERT_EQ(Decimal64("632098759743209472"), Decimal64("1234567890123456") << 9); check(-123, 0); check(-123, 1); @@ -1866,7 +1875,8 @@ TEST(Decimal64Test, LeftShift) { ASSERT_EQ(Decimal64("-1234567890123456"), Decimal64("-1234567890123456") << 0); ASSERT_EQ(Decimal64("-2469135780246912"), Decimal64("-1234567890123456") << 1); - ASSERT_EQ(Decimal64("-6917529027641081856"), Decimal64("-1234567890123456") << 55); + // shift 9 keeps the result within kMaxPrecision (18 digits) (GH-49817) + ASSERT_EQ(Decimal64("-632098759743209472"), Decimal64("-1234567890123456") << 9); } TEST(Decimal64Test, RightShift) { @@ -1932,8 +1942,9 @@ TEST(Decimal128Test, LeftShift) { ASSERT_EQ(Decimal128("199999999999998"), Decimal128("99999999999999") << 1); ASSERT_EQ(Decimal128("3435973836799965640261632"), Decimal128("99999999999999") << 35); - ASSERT_EQ(Decimal128("120892581961461708544797985370825293824"), - Decimal128("99999999999999") << 80); + // shift 79 keeps the result within kMaxPrecision (38 digits) (GH-49817) + ASSERT_EQ(Decimal128("60446290980730854272398992685412646912"), + Decimal128("99999999999999") << 79); ASSERT_EQ(Decimal128("1234567890123456789012"), Decimal128("1234567890123456789012") << 0); @@ -1951,8 +1962,9 @@ TEST(Decimal128Test, LeftShift) { ASSERT_EQ(Decimal128("-199999999999998"), Decimal128("-99999999999999") << 1); ASSERT_EQ(Decimal128("-3435973836799965640261632"), Decimal128("-99999999999999") << 35); - ASSERT_EQ(Decimal128("-120892581961461708544797985370825293824"), - Decimal128("-99999999999999") << 80); + // shift 79 keeps the result within kMaxPrecision (38 digits) (GH-49817) + ASSERT_EQ(Decimal128("-60446290980730854272398992685412646912"), + Decimal128("-99999999999999") << 79); ASSERT_EQ(Decimal128("-1234567890123456789012"), Decimal128("-1234567890123456789012") << 0);