From b92c4c84c9ff371e5e909df384c871ee6f131266 Mon Sep 17 00:00:00 2001 From: Mar Date: Sun, 18 Oct 2026 11:18:12 +0200 Subject: [PATCH] GH-55340: [C++] Check out-of-range decimal scales before indexing powers-of-ten tables A decimal type's scale can be any int32_t value and the scale of a decimal string comes from its exponent, but IncreaseScaleBy(), ReduceScaleBy(), GetWholeAndFraction() and the floating-point PowerOfTen() index fixed-size tables and only check the index with a DCHECK. - Truncating casts from decimal to integer, decimal to decimal and string to decimal bound the scale change: reducing it by more than kMaxScale gives zero, increasing it by more than kMaxScale is an error for a non-zero value. - product and hash_product reject a decimal scale outside [0, kMaxScale]. - Decimal ToReal() does not split the value into integer and fractional parts above kMaxScale, and FromReal() uses the approximate algorithm with LargePowerOfTen() there. --- cpp/src/arrow/acero/hash_aggregate_test.cc | 22 +++++ .../arrow/compute/kernels/aggregate_basic.cc | 2 + .../compute/kernels/aggregate_internal.h | 13 +++ .../arrow/compute/kernels/aggregate_test.cc | 17 ++++ .../compute/kernels/hash_aggregate_numeric.cc | 5 ++ .../compute/kernels/scalar_cast_numeric.cc | 63 +++++++++++--- .../arrow/compute/kernels/scalar_cast_test.cc | 85 +++++++++++++++++++ cpp/src/arrow/util/decimal.cc | 53 ++++++------ cpp/src/arrow/util/decimal_test.cc | 37 ++++++++ 9 files changed, 259 insertions(+), 38 deletions(-) diff --git a/cpp/src/arrow/acero/hash_aggregate_test.cc b/cpp/src/arrow/acero/hash_aggregate_test.cc index 2f498f3a7bb2..29f299b06f0a 100644 --- a/cpp/src/arrow/acero/hash_aggregate_test.cc +++ b/cpp/src/arrow/acero/hash_aggregate_test.cc @@ -956,6 +956,28 @@ TEST_P(GroupBy, SumMeanProductDecimal) { } } +TEST_P(GroupBy, ProductDecimalScaleOutOfRange) { + // GH-55340: the product is rescaled by the decimal type's scale, which must be + // within [0, kMaxScale] + auto key = ArrayFromJSON(int64(), "[1, 2]"); + for (const auto& [unscaled_type, max_scale] : + std::vector, int32_t>>{ + {decimal128(38, 0), 38}, {decimal256(76, 0), 76}}) { + auto unscaled = ArrayFromJSON(unscaled_type, R"(["2", "3"])"); + for (int32_t scale : {-1, max_scale + 1}) { + ASSERT_OK_AND_ASSIGN(auto type, + DecimalType::Make(unscaled_type->id(), max_scale, scale)); + ASSERT_OK_AND_ASSIGN(auto values, unscaled->View(type)); + for (bool use_threads : {true, false}) { + SCOPED_TRACE(use_threads ? "parallel/merged" : "serial"); + EXPECT_RAISES_WITH_MESSAGE_THAT( + NotImplemented, HasSubstr("scale outside"), + GroupByTest({values}, {key}, {{"hash_product", nullptr}}, use_threads)); + } + } + } +} + TEST_P(GroupBy, MeanOnly) { for (bool use_threads : {true, false}) { SCOPED_TRACE(use_threads ? "parallel/merged" : "serial"); diff --git a/cpp/src/arrow/compute/kernels/aggregate_basic.cc b/cpp/src/arrow/compute/kernels/aggregate_basic.cc index f7ff72bc114a..4f9a43b31acf 100644 --- a/cpp/src/arrow/compute/kernels/aggregate_basic.cc +++ b/cpp/src/arrow/compute/kernels/aggregate_basic.cc @@ -425,6 +425,8 @@ struct ProductInit { template enable_if_decimal Visit(const Type&) { + using AccType = typename ProductImpl::AccType; + RETURN_NOT_OK(MultiplyTraits::CheckType(*type)); state.reset(new ProductImpl(type, options)); return Status::OK(); } diff --git a/cpp/src/arrow/compute/kernels/aggregate_internal.h b/cpp/src/arrow/compute/kernels/aggregate_internal.h index 126f2891e212..85d1a72aa725 100644 --- a/cpp/src/arrow/compute/kernels/aggregate_internal.h +++ b/cpp/src/arrow/compute/kernels/aggregate_internal.h @@ -79,6 +79,8 @@ template struct MultiplyTraits { using CType = typename TypeTraits::CType; + static Status CheckType(const DataType&) { return Status::OK(); } + constexpr static CType one(const DataType&) { return static_cast(1); } constexpr static CType Multiply(const DataType&, CType lhs, CType rhs) { @@ -90,6 +92,17 @@ template struct MultiplyTraits> { using CType = typename TypeTraits::CType; + // one() and Multiply() rescale by the type's scale, which IncreaseScaleBy() + // and ReduceScaleBy() only accept in [0, kMaxScale] + static Status CheckType(const DataType& ty) { + const int32_t scale = static_cast(ty).scale(); + if (scale < 0 || scale > CType::kMaxScale) { + return Status::NotImplemented("Product of decimal type with scale outside [0, ", + CType::kMaxScale, "]: ", ty); + } + return Status::OK(); + } + constexpr static CType one(const DataType& ty) { // Return 1 scaled to output type scale return CType(1).IncreaseScaleBy(static_cast(ty).scale()); diff --git a/cpp/src/arrow/compute/kernels/aggregate_test.cc b/cpp/src/arrow/compute/kernels/aggregate_test.cc index 4ef2b2f88a51..104743f38ab1 100644 --- a/cpp/src/arrow/compute/kernels/aggregate_test.cc +++ b/cpp/src/arrow/compute/kernels/aggregate_test.cc @@ -846,6 +846,23 @@ TEST(TestDecimalProductKernel, ScalarAggregateOptions) { } } +TEST(TestDecimalProductKernel, ScaleOutOfRange) { + // GH-55340: the product is rescaled by the decimal type's scale, which must be + // within [0, kMaxScale] + for (const auto& [unscaled_type, max_scale] : + std::vector, int32_t>>{ + {decimal128(38, 0), 38}, {decimal256(76, 0), 76}}) { + auto unscaled = ArrayFromJSON(unscaled_type, R"(["2", "3"])"); + for (int32_t scale : {-1, max_scale + 1}) { + ASSERT_OK_AND_ASSIGN(auto type, + DecimalType::Make(unscaled_type->id(), max_scale, scale)); + ASSERT_OK_AND_ASSIGN(auto values, unscaled->View(type)); + EXPECT_RAISES_WITH_MESSAGE_THAT( + NotImplemented, ::testing::HasSubstr("scale outside"), Product(values)); + } + } +} + TEST(TestProductKernel, Overflow) { EXPECT_THAT(Product(ArrayFromJSON(int64(), "[8589934592, 8589934593]")), ResultWith(Datum(static_cast(8589934592)))); diff --git a/cpp/src/arrow/compute/kernels/hash_aggregate_numeric.cc b/cpp/src/arrow/compute/kernels/hash_aggregate_numeric.cc index 30504ba87b6b..cfce85fe9646 100644 --- a/cpp/src/arrow/compute/kernels/hash_aggregate_numeric.cc +++ b/cpp/src/arrow/compute/kernels/hash_aggregate_numeric.cc @@ -315,6 +315,11 @@ struct GroupedProductImpl final using CType = typename Base::CType; using InputCType = typename Base::InputCType; + Status Init(ExecContext* ctx, const KernelInitArgs& args) override { + RETURN_NOT_OK(MultiplyTraits::CheckType(*args.inputs[0])); + return Base::Init(ctx, args); + } + static CType NullValue(const DataType& out_type) { return MultiplyTraits::one(out_type); } diff --git a/cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc b/cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc index ba2a8c4b5638..1d94fedc9614 100644 --- a/cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc +++ b/cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc @@ -335,6 +335,38 @@ struct CastFunctor> { } }; +// ---------------------------------------------------------------------- +// Decimal rescaling for truncating casts + +// IncreaseScaleBy() and ReduceScaleBy() look up a table of powers of ten and only +// accept a scale change in [0, kMaxScale]. A scale change computed from decimal +// types or from a parsed decimal string is not bounded this way (a decimal type's +// scale can be any int32_t value), so it must be checked first. + +template +Decimal UnsafeIncreaseScaleBy(const Decimal& val, int64_t increase_by, Status* st) { + DCHECK_GE(increase_by, 0); + if (ARROW_PREDICT_FALSE(increase_by > Decimal::kMaxScale)) { + // Any non-zero value overflows + if (val != Decimal{}) { + *st = Status::Invalid("Rescaling decimal value by ", increase_by, + " digits would overflow"); + } + return Decimal{}; + } + return val.IncreaseScaleBy(static_cast(increase_by)); +} + +template +Decimal UnsafeReduceScaleBy(const Decimal& val, int64_t reduce_by) { + DCHECK_GE(reduce_by, 0); + if (ARROW_PREDICT_FALSE(reduce_by > Decimal::kMaxScale)) { + // Any value is less than 10^(kMaxScale + 1) in magnitude, so it truncates to zero + return Decimal{}; + } + return val.ReduceScaleBy(static_cast(reduce_by), /*round=*/false); +} + // ---------------------------------------------------------------------- // Decimal to integer @@ -364,7 +396,8 @@ struct UnsafeUpscaleDecimalToInteger : public DecimalToIntegerMixin { template OutValue Call(KernelContext* ctx, Arg0Value val, Status* st) const { - return ToInteger(ctx, val.IncreaseScaleBy(-in_scale_), st); + return ToInteger( + ctx, UnsafeIncreaseScaleBy(val, -static_cast(in_scale_), st), st); } }; @@ -373,7 +406,7 @@ struct UnsafeDownscaleDecimalToInteger : public DecimalToIntegerMixin { template OutValue Call(KernelContext* ctx, Arg0Value val, Status* st) const { - return ToInteger(ctx, val.ReduceScaleBy(in_scale_, false), st); + return ToInteger(ctx, UnsafeReduceScaleBy(val, in_scale_), st); } }; @@ -553,11 +586,12 @@ struct DecimalConversions { struct UnsafeUpscaleDecimal { template - OutValue Call(KernelContext*, Arg0Value val, Status*) const { + OutValue Call(KernelContext*, Arg0Value val, Status* st) const { using Conv = DecimalConversions; - return Conv::ConvertOutput(Conv::ConvertInput(std::move(val)).IncreaseScaleBy(by_)); + return Conv::ConvertOutput( + UnsafeIncreaseScaleBy(Conv::ConvertInput(std::move(val)), by_, st)); } - int32_t by_; + int64_t by_; }; struct UnsafeDownscaleDecimal { @@ -565,9 +599,9 @@ struct UnsafeDownscaleDecimal { OutValue Call(KernelContext*, Arg0Value val, Status*) const { using Conv = DecimalConversions; return Conv::ConvertOutput( - Conv::ConvertInput(std::move(val)).ReduceScaleBy(by_, false)); + UnsafeReduceScaleBy(Conv::ConvertInput(std::move(val)), by_)); } - int32_t by_; + int64_t by_; }; struct SafeRescaleDecimal { @@ -604,15 +638,17 @@ struct CastFunctor(out_scale) - in_scale; + if (delta_scale > 0) { // Unsafe upscale applicator::ScalarUnaryNotNullStateful kernel( - UnsafeUpscaleDecimal{out_scale - in_scale}); + UnsafeUpscaleDecimal{delta_scale}); return kernel.Exec(ctx, batch, out); } else { // Unsafe downscale applicator::ScalarUnaryNotNullStateful kernel( - UnsafeDownscaleDecimal{in_scale - out_scale}); + UnsafeDownscaleDecimal{-delta_scale}); return kernel.Exec(ctx, batch, out); } } @@ -675,9 +711,10 @@ struct StringToDecimal { if (ARROW_PREDICT_TRUE(r_parse.ok())) { if (allow_truncate_) { - return (parsed_scale < out_scale_) - ? parsed_out.IncreaseScaleBy(out_scale_ - parsed_scale) - : parsed_out.ReduceScaleBy(parsed_scale - out_scale_, false); + // The parsed scale comes from the string's exponent and can be arbitrarily large + const int64_t delta_scale = static_cast(out_scale_) - parsed_scale; + return (delta_scale > 0) ? UnsafeIncreaseScaleBy(parsed_out, delta_scale, st) + : UnsafeReduceScaleBy(parsed_out, -delta_scale); } auto maybe_rescaled = parsed_out.Rescale(parsed_scale, out_scale_); diff --git a/cpp/src/arrow/compute/kernels/scalar_cast_test.cc b/cpp/src/arrow/compute/kernels/scalar_cast_test.cc index 864ec4afce0c..0c105e31142e 100644 --- a/cpp/src/arrow/compute/kernels/scalar_cast_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_cast_test.cc @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -83,6 +84,20 @@ static std::vector> kBaseBinaryTypes = { static std::vector> kBaseBinaryAndViewTypes = { binary(), utf8(), large_binary(), large_utf8(), utf8_view(), binary_view()}; +// The decimal type of the same width and precision with another scale, which may be +// out of the range that DecimalXXX::FromString() can produce +static std::shared_ptr DecimalTypeWithScale( + const std::shared_ptr& type, int32_t scale) { + const auto& decimal_type = checked_cast(*type); + return DecimalType::Make(type->id(), decimal_type.precision(), scale).ValueOrDie(); +} + +// The same unscaled decimal values with another scale +static std::shared_ptr DecimalViewWithScale(const std::shared_ptr& array, + int32_t scale) { + return array->View(DecimalTypeWithScale(array->type(), scale)).ValueOrDie(); +} + static void AssertBufferSame(const Array& left, const Array& right, size_t buffer_index) { ASSERT_EQ(left.data()->buffers[buffer_index].get(), right.data()->buffers[buffer_index].get()); @@ -764,6 +779,33 @@ TEST(Cast, Decimal256ToInt) { CheckCast(negative_scale, ArrayFromJSON(int64(), "[1234567890000, -120000]"), options); } +TEST(Cast, DecimalToIntTruncateScaleOutOfRange) { + // GH-55340: a decimal type's scale is not bounded by kMaxScale, so neither is the + // scale change of a truncating cast + auto options = CastOptions::Unsafe(int64()); + for (const auto& [unscaled_type, max_scale] : + std::vector, int32_t>>{ + {decimal32(9, 0), 9}, + {decimal64(18, 0), 18}, + {decimal128(38, 0), 38}, + {decimal256(76, 0), 76}}) { + auto unscaled = ArrayFromJSON(unscaled_type, R"(["12", "-3", "0", null])"); + auto zero = ArrayFromJSON(unscaled_type, R"(["0", null])"); + // Downscaling truncates to zero + for (int32_t scale : + {max_scale, max_scale + 1, std::numeric_limits::max()}) { + CheckCast(DecimalViewWithScale(unscaled, scale), + ArrayFromJSON(int64(), "[0, 0, 0, null]"), options); + } + // Upscaling overflows, except for zero + for (int32_t scale : {-max_scale - 1, std::numeric_limits::min()}) { + CheckCastFails(DecimalViewWithScale(unscaled, scale), options); + CheckCast(DecimalViewWithScale(zero, scale), ArrayFromJSON(int64(), "[0, null]"), + options); + } + } +} + TEST(Cast, IntegerToDecimal) { for (auto decimal_type : {decimal32(9, 2), decimal64(18, 2), decimal128(22, 2), decimal256(22, 2)}) { @@ -2145,6 +2187,31 @@ TEST(Cast, Decimal256ToDecimal128) { } } +TEST(Cast, DecimalToDecimalTruncateScaleOutOfRange) { + // GH-55340: a decimal type's scale is not bounded by kMaxScale, so neither is the + // scale change of a truncating cast + auto options = CastOptions::Unsafe(); + const std::vector> unscaled_types = { + decimal32(9, 0), decimal64(18, 0), decimal128(38, 0), decimal256(76, 0)}; + for (const auto& in_type : unscaled_types) { + auto unscaled = ArrayFromJSON(in_type, R"(["12", "-3", "0", null])"); + for (const auto& out_type : unscaled_types) { + // Downscaling by more than kMaxScale truncates to zero + for (int32_t scale : {77, std::numeric_limits::max()}) { + CheckCast(DecimalViewWithScale(unscaled, scale), + ArrayFromJSON(out_type, R"(["0", "0", "0", null])"), options); + } + // Upscaling by more than kMaxScale overflows + options.to_type = DecimalTypeWithScale(out_type, 77); + CheckCastFails(unscaled, options); + options.to_type = + DecimalTypeWithScale(out_type, std::numeric_limits::max()); + CheckCastFails(DecimalViewWithScale(unscaled, std::numeric_limits::min()), + options); + } + } +} + TEST(Cast, FloatingToDecimal) { for (auto float_type : {float32(), float64()}) { for (auto decimal_type : @@ -3133,6 +3200,24 @@ TEST(Cast, StringToDecimal) { } } +TEST(Cast, StringToDecimalTruncateScaleOutOfRange) { + // GH-55340: the scale of a decimal string comes from its exponent and is not + // bounded by kMaxScale + auto options = CastOptions::Unsafe(); + for (auto string_type : {utf8(), large_utf8()}) { + for (auto decimal_type : + {decimal32(9, 0), decimal64(18, 0), decimal128(38, 0), decimal256(76, 0)}) { + // Downscaling by more than kMaxScale truncates to zero + CheckCast(ArrayFromJSON(string_type, + R"(["1E-77", "-12E-1000000000", "1E-2147483647", null])"), + ArrayFromJSON(decimal_type, R"(["0", "0", "0", null])"), options); + // Upscaling by more than kMaxScale overflows + options.to_type = DecimalTypeWithScale(decimal_type, 77); + CheckCastFails(ArrayFromJSON(string_type, R"(["1"])"), options); + } + } +} + TEST(Cast, StringToTimestamp) { for (auto string_type : {utf8(), large_utf8()}) { auto strings = ArrayFromJSON(string_type, R"(["1970-01-01", null, "2000-02-29"])"); diff --git a/cpp/src/arrow/util/decimal.cc b/cpp/src/arrow/util/decimal.cc index a9d2fcb02d94..26aef34e97d2 100644 --- a/cpp/src/arrow/util/decimal.cc +++ b/cpp/src/arrow/util/decimal.cc @@ -81,7 +81,7 @@ struct BaseDecimalRealConversion { // Return 10**exp, with a fast lookup if possible template - static Real LargePowerOfTen(int32_t exp) { + static Real LargePowerOfTen(int64_t exp) { constexpr int N = kPrecomputedPowersOfTen; if (ARROW_PREDICT_TRUE(exp >= -N && exp <= N)) { return RealTraits::powers_of_ten()[exp + N]; @@ -124,8 +124,9 @@ struct DecimalRealConversion : public BaseDecimalRealConversion { // Problem statement: construct the Decimal with the value // closest to `real * 10^scale`. - if (scale < 0) { - // Negative scales are not handled below, fall back to approx algorithm + if (scale < 0 || scale > kMaxScale) { + // Negative scales and scales too large for the powers of ten tables + // are not handled below, fall back to approx algorithm return Derived::FromPositiveRealApprox(real, precision, scale); } @@ -242,8 +243,6 @@ struct DecimalRealConversion : public BaseDecimalRealConversion { static Result FromReal(Real x, int32_t precision, int32_t scale) { DCHECK_GT(precision, 0); DCHECK_LE(precision, kMaxPrecision); - DCHECK_GE(scale, -kMaxScale); - DCHECK_LE(scale, kMaxScale); if (!std::isfinite(x)) { return Status::Invalid("Cannot convert ", x, " to Decimal128"); @@ -261,8 +260,6 @@ struct DecimalRealConversion : public BaseDecimalRealConversion { template static Real ToReal(const DecimalType& decimal, int32_t scale) { - DCHECK_GE(scale, -kMaxScale); - DCHECK_LE(scale, kMaxScale); if (decimal.IsNegative()) { // Convert the absolute value to avoid precision loss auto abs = decimal; @@ -290,7 +287,7 @@ struct Decimal32RealConversion template static Result FromPositiveRealApprox(Real real, int32_t precision, int32_t scale) { - const auto x = std::nearbyint(real * PowerOfTen(scale)); + const auto x = std::nearbyint(real * LargePowerOfTen(scale)); const auto max_abs = PowerOfTen(precision); if (x <= -max_abs || x >= max_abs) { return OverflowError(real, precision, scale); @@ -302,13 +299,14 @@ struct Decimal32RealConversion template static Real ToRealPositiveNoSplit(const Decimal32& decimal, int32_t scale) { Real x = static_cast(decimal.value()); - x *= LargePowerOfTen(-scale); + x *= LargePowerOfTen(-static_cast(scale)); return x; } template static Real ToRealPositive(const Decimal32& decimal, int32_t scale) { - if (scale <= 0 || uint64_t(decimal.value()) <= RealTraits::kMaxPreciseInteger) { + if (scale <= 0 || scale > kMaxScale || + uint64_t(decimal.value()) <= RealTraits::kMaxPreciseInteger) { return ToRealPositiveNoSplit(decimal, scale); } @@ -357,7 +355,7 @@ struct Decimal64RealConversion template static Result FromPositiveRealApprox(Real real, int32_t precision, int32_t scale) { - const auto x = std::nearbyint(real * PowerOfTen(scale)); + const auto x = std::nearbyint(real * LargePowerOfTen(scale)); const auto max_abs = PowerOfTen(precision); if (x <= -max_abs || x >= max_abs) { return OverflowError(real, precision, scale); @@ -369,13 +367,14 @@ struct Decimal64RealConversion template static Real ToRealPositiveNoSplit(const Decimal64& decimal, int32_t scale) { Real x = static_cast(decimal.value()); - x *= LargePowerOfTen(-scale); + x *= LargePowerOfTen(-static_cast(scale)); return x; } template static Real ToRealPositive(const Decimal64& decimal, int32_t scale) { - if (scale <= 0 || uint64_t(decimal.value()) <= RealTraits::kMaxPreciseInteger) { + if (scale <= 0 || scale > kMaxScale || + uint64_t(decimal.value()) <= RealTraits::kMaxPreciseInteger) { return ToRealPositiveNoSplit(decimal, scale); } @@ -440,7 +439,7 @@ struct Decimal128RealConversion int32_t scale) { // Approximate algorithm that operates in the FP domain (thus subject // to precision loss). - const auto x = std::nearbyint(real * PowerOfTen(scale)); + const auto x = std::nearbyint(real * LargePowerOfTen(scale)); const auto max_abs = PowerOfTen(precision); if (x <= -max_abs || x >= max_abs) { return OverflowError(real, precision, scale); @@ -460,7 +459,7 @@ struct Decimal128RealConversion static Real ToRealPositiveNoSplit(const Decimal128& decimal, int32_t scale) { Real x = RealTraits::two_to_64(static_cast(decimal.high_bits())); x += static_cast(decimal.low_bits()); - x *= LargePowerOfTen(-scale); + x *= LargePowerOfTen(-static_cast(scale)); return x; } @@ -474,10 +473,12 @@ struct Decimal128RealConversion /// Here "exact value" means the closest representable value by Real. template static Real ToRealPositive(const Decimal128& decimal, int32_t scale) { - if (scale <= 0 || (decimal.high_bits() == 0 && - decimal.low_bits() <= RealTraits::kMaxPreciseInteger)) { - // No need to split the decimal if it is already an integer (scale <= 0) or if it - // can be precisely represented by Real + if (scale <= 0 || scale > kMaxScale || + (decimal.high_bits() == 0 && + decimal.low_bits() <= RealTraits::kMaxPreciseInteger)) { + // No need to split the decimal if it is already an integer (scale <= 0), if it + // has no integer part (scale > kMaxScale) or if it can be precisely represented + // by Real return ToRealPositiveNoSplit(decimal, scale); } @@ -1363,7 +1364,7 @@ struct Decimal256RealConversion template static Result FromPositiveRealApprox(Real real, int32_t precision, int32_t scale) { - auto x = std::nearbyint(real * PowerOfTen(scale)); + auto x = std::nearbyint(real * LargePowerOfTen(scale)); const auto max_abs = PowerOfTen(precision); if (x >= max_abs) { return OverflowError(real, precision, scale); @@ -1399,7 +1400,7 @@ struct Decimal256RealConversion x += RealTraits::two_to_128(static_cast(parts_le[2])); x += RealTraits::two_to_64(static_cast(parts_le[1])); x += static_cast(parts_le[0]); - x *= LargePowerOfTen(-scale); + x *= LargePowerOfTen(-static_cast(scale)); return x; } @@ -1414,10 +1415,12 @@ struct Decimal256RealConversion template static Real ToRealPositive(const Decimal256& decimal, int32_t scale) { const auto parts_le = decimal.little_endian_array(); - if (scale <= 0 || (parts_le[3] == 0 && parts_le[2] == 0 && parts_le[1] == 0 && - parts_le[0] < RealTraits::kMaxPreciseInteger)) { - // No need to split the decimal if it is already an integer (scale <= 0) or if it - // can be precisely represented by Real + if (scale <= 0 || scale > kMaxScale || + (parts_le[3] == 0 && parts_le[2] == 0 && parts_le[1] == 0 && + parts_le[0] < RealTraits::kMaxPreciseInteger)) { + // No need to split the decimal if it is already an integer (scale <= 0), if it + // has no integer part (scale > kMaxScale) or if it can be precisely represented + // by Real return ToRealPositiveNoSplit(decimal, scale); } diff --git a/cpp/src/arrow/util/decimal_test.cc b/cpp/src/arrow/util/decimal_test.cc index 7022c8117802..927d6ce5af60 100644 --- a/cpp/src/arrow/util/decimal_test.cc +++ b/cpp/src/arrow/util/decimal_test.cc @@ -899,6 +899,19 @@ class TestDecimalFromReal : public ::testing::Test { ASSERT_RAISES(Invalid, Decimal::FromReal(-1e+36, 36, 0)); } } + + void TestScaleOutOfRange() { + // GH-55340: a decimal type's scale is not bounded by kMaxScale + constexpr int32_t kPrecision = Decimal::kMaxPrecision; + // Overflows at a very large scale + ASSERT_RAISES(Invalid, Decimal::FromReal(Real{1}, kPrecision, 1000)); + ASSERT_RAISES(Invalid, Decimal::FromReal(Real{1}, kPrecision, + std::numeric_limits::max())); + // Rounds to zero at a very small scale + CheckDecimalFromRealIntegerString(Real{1}, kPrecision, -1000, "0"); + CheckDecimalFromRealIntegerString(Real{1}, kPrecision, + std::numeric_limits::min(), "0"); + } }; using RealTypes = @@ -912,6 +925,8 @@ TYPED_TEST(TestDecimalFromReal, TestSuccess) { this->TestSuccess(); } TYPED_TEST(TestDecimalFromReal, TestErrors) { this->TestErrors(); } +TYPED_TEST(TestDecimalFromReal, TestScaleOutOfRange) { this->TestScaleOutOfRange(); } + // Tests for Decimal128::FromReal(float, ...) and Decimal256::FromReal(float, ...) template class TestDecimalFromRealFloat : public ::testing::Test { @@ -1206,10 +1221,32 @@ class TestDecimalToReal : public ::testing::Test { } } } + + void TestScaleOutOfRange() { + // GH-55340: a decimal type's scale is not bounded by kMaxScale + // A value not precisely representable by Real, so that it is not converted at once + const std::string value = + std::is_same_v ? "123456789" : "123456789012345678"; + if (value.size() <= Decimal::kMaxPrecision) { + // Above kMaxScale, a decimal value has no integer part + for (int32_t scale : {Decimal::kMaxScale + 1, Decimal::kMaxScale + 20}) { +#ifdef __MINGW32__ // MinGW has precision issues + if constexpr (std::is_same_v) continue; +#endif + const Real expected = static_cast(std::stod(value)) * Pow10(-scale); + CheckDecimalToRealApprox(value, scale, expected); + CheckDecimalToRealApprox("-" + value, scale, -expected); + } + CheckDecimalToReal(value, std::numeric_limits::max(), 0); + } + CheckDecimalToReal("1", std::numeric_limits::min(), + std::numeric_limits::infinity()); + } }; TYPED_TEST_SUITE(TestDecimalToReal, RealTypes); TYPED_TEST(TestDecimalToReal, TestSuccess) { this->TestSuccess(); } +TYPED_TEST(TestDecimalToReal, TestScaleOutOfRange) { this->TestScaleOutOfRange(); } // Custom test for Decimal::ToReal template