From 797704211760d27e4a57e65facfa6fe67511cc70 Mon Sep 17 00:00:00 2001 From: Sai Asish Y Date: Tue, 21 Apr 2026 16:07:31 -0700 Subject: [PATCH 1/3] 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); From 7c3128855249747fb309b83cca4d81c56b441421 Mon Sep 17 00:00:00 2001 From: Sai Asish Y Date: Thu, 11 Jun 2026 12:11:27 -0700 Subject: [PATCH 2/3] GH-49817: [C++] Test max decimal value and clarify shift comments Signed-off-by: Sai Asish Y --- cpp/src/arrow/util/decimal_test.cc | 47 ++++++++++++++++++------------ 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/cpp/src/arrow/util/decimal_test.cc b/cpp/src/arrow/util/decimal_test.cc index 5e124c343e21..7cc90df467db 100644 --- a/cpp/src/arrow/util/decimal_test.cc +++ b/cpp/src/arrow/util/decimal_test.cc @@ -711,19 +711,23 @@ TEST(Decimal128Test, PrintLargeNegativeValue) { } TEST(Decimal128Test, PrintMaxValue) { - // 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); + // 99999...9 is the largest value that fits in kMaxPrecision (38) digits; the + // truncated bit-pattern max is also exercised (GH-49817). + for (const auto& string_value : + {std::string("99999999999999999999999999999999999999"), + std::string("17014118346046923173168730371588410572")}) { + const Decimal128 value(string_value); + ASSERT_EQ(string_value, value.ToIntegerString()); + } } TEST(Decimal128Test, PrintMinValue) { - // 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); + for (const auto& string_value : + {std::string("-99999999999999999999999999999999999999"), + std::string("-17014118346046923173168730371588410572")}) { + const Decimal128 value(string_value); + ASSERT_EQ(string_value, value.ToIntegerString()); + } } struct ToStringTestParam { @@ -1745,8 +1749,9 @@ TYPED_TEST(TestBasicDecimalFunctionality, FitsInPrecision) { ASSERT_TRUE(TypeParam(max_nines).FitsInPrecision(TypeParam::kMaxPrecision)); ASSERT_TRUE(TypeParam("-" + max_nines).FitsInPrecision(TypeParam::kMaxPrecision)); - // construct 10^kMaxPrecision without going through the string parser (GH-49817) - TypeParam one_past_max = TypeParam::GetMaxValue(TypeParam::kMaxPrecision) + 1; + // max_nines + 1 is 10^kMaxPrecision, the first value that no longer fits; + // build it arithmetically since the string form is rejected now (GH-49817) + TypeParam one_past_max = TypeParam(max_nines) + 1; ASSERT_FALSE(one_past_max.FitsInPrecision(TypeParam::kMaxPrecision)); ASSERT_FALSE((-one_past_max).FitsInPrecision(TypeParam::kMaxPrecision)); } @@ -1769,7 +1774,8 @@ TEST(Decimal32Test, LeftShift) { check(123, 16); check(123, 30); - // results trimmed to kMaxPrecision (9) digits (GH-49817) + // operands picked so the expected results fit in kMaxPrecision (9) digits, + // since FromString now rejects wider literals (GH-49817) ASSERT_EQ(Decimal32("999999998"), Decimal32("499999999") << 1); ASSERT_EQ(Decimal32("12799872"), Decimal32("99999") << 7); ASSERT_EQ(Decimal32("819191808"), Decimal32("99999") << 13); @@ -1784,7 +1790,8 @@ TEST(Decimal32Test, LeftShift) { check(-123, 16); check(-123, 30); - // results trimmed to kMaxPrecision (9) digits (GH-49817) + // operands picked so the expected results fit in kMaxPrecision (9) digits, + // since FromString now rejects wider literals (GH-49817) ASSERT_EQ(Decimal32("-999999998"), Decimal32("-499999999") << 1); ASSERT_EQ(Decimal32("-12799872"), Decimal32("-99999") << 7); ASSERT_EQ(Decimal32("-819191808"), Decimal32("-99999") << 13); @@ -1860,7 +1867,8 @@ TEST(Decimal64Test, LeftShift) { ASSERT_EQ(Decimal64("1234567890123456"), Decimal64("1234567890123456") << 0); ASSERT_EQ(Decimal64("2469135780246912"), Decimal64("1234567890123456") << 1); - // shift 9 keeps the result within kMaxPrecision (18 digits) (GH-49817) + // shift amount picked so the expected result fits in kMaxPrecision (18 + // digits), since FromString now rejects wider literals (GH-49817) ASSERT_EQ(Decimal64("632098759743209472"), Decimal64("1234567890123456") << 9); check(-123, 0); @@ -1875,7 +1883,8 @@ TEST(Decimal64Test, LeftShift) { ASSERT_EQ(Decimal64("-1234567890123456"), Decimal64("-1234567890123456") << 0); ASSERT_EQ(Decimal64("-2469135780246912"), Decimal64("-1234567890123456") << 1); - // shift 9 keeps the result within kMaxPrecision (18 digits) (GH-49817) + // shift amount picked so the expected result fits in kMaxPrecision (18 + // digits), since FromString now rejects wider literals (GH-49817) ASSERT_EQ(Decimal64("-632098759743209472"), Decimal64("-1234567890123456") << 9); } @@ -1942,7 +1951,8 @@ TEST(Decimal128Test, LeftShift) { ASSERT_EQ(Decimal128("199999999999998"), Decimal128("99999999999999") << 1); ASSERT_EQ(Decimal128("3435973836799965640261632"), Decimal128("99999999999999") << 35); - // shift 79 keeps the result within kMaxPrecision (38 digits) (GH-49817) + // shift amount picked so the expected result fits in kMaxPrecision (38 + // digits), since FromString now rejects wider literals (GH-49817) ASSERT_EQ(Decimal128("60446290980730854272398992685412646912"), Decimal128("99999999999999") << 79); @@ -1962,7 +1972,8 @@ TEST(Decimal128Test, LeftShift) { ASSERT_EQ(Decimal128("-199999999999998"), Decimal128("-99999999999999") << 1); ASSERT_EQ(Decimal128("-3435973836799965640261632"), Decimal128("-99999999999999") << 35); - // shift 79 keeps the result within kMaxPrecision (38 digits) (GH-49817) + // shift amount picked so the expected result fits in kMaxPrecision (38 + // digits), since FromString now rejects wider literals (GH-49817) ASSERT_EQ(Decimal128("-60446290980730854272398992685412646912"), Decimal128("-99999999999999") << 79); From 7761dd6f80eab5a1aac2642649d88ec480896257 Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Tue, 16 Jun 2026 11:21:26 +0200 Subject: [PATCH 3/3] Fix lint --- cpp/src/arrow/util/decimal_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cpp/src/arrow/util/decimal_test.cc b/cpp/src/arrow/util/decimal_test.cc index 7cc90df467db..79cd11474fd0 100644 --- a/cpp/src/arrow/util/decimal_test.cc +++ b/cpp/src/arrow/util/decimal_test.cc @@ -325,7 +325,8 @@ TEST(Decimal128Test, TestStringRoundTrip) { 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); + 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);