From f4387eed2e92f7f768dd3752ae25fae9d3c6eed7 Mon Sep 17 00:00:00 2001 From: Soki Fukuda Date: Tue, 13 Oct 2026 02:37:06 +0900 Subject: [PATCH 1/2] GH-53288: [C++] Rescale and range-check before narrowing decimals to Decimal32 DecimalConversions follows a convention its own comments state: widening conversions "convert then scale", narrowing ones "scale then truncate". SafeRescaleDecimal relies on it, calling ConvertInput -> Rescale -> FitsInPrecision -> ConvertOutput, so on every narrowing path the overflow check sees the full-width coefficient. DecimalConversions is the only specialization that narrows in ConvertInput, and it is the single implementation behind every Decimal{32,64,128,256} -> Decimal32 cast, so both Rescale and FitsInPrecision ran against an already-truncated coefficient. Give it the same shape as DecimalConversions: ConvertInput keeps InDecimal and ConvertOutput narrows. low_bits() is defined on SmallBasicDecimal as well as on BasicDecimal128/BasicDecimal256, so one generic specialization still covers all four source types. --- .../compute/kernels/scalar_cast_numeric.cc | 8 ++++-- .../arrow/compute/kernels/scalar_cast_test.cc | 28 +++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc b/cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc index ba2a8c4b5638..66d4d5985cad 100644 --- a/cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc +++ b/cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc @@ -486,8 +486,12 @@ struct DecimalConversions { template struct DecimalConversions { - static Decimal32 ConvertInput(InDecimal&& val) { return Decimal32(val.low_bits()); } - static Decimal32 ConvertOutput(Decimal32&& val) { return val; } + // Scale then truncate. Narrowing in ConvertInput would drop coefficient bits + // before Rescale and FitsInPrecision could see them. + static InDecimal ConvertInput(InDecimal&& val) { return val; } + static Decimal32 ConvertOutput(InDecimal&& val) { + return Decimal32(static_cast(val.low_bits())); + } }; template <> diff --git a/cpp/src/arrow/compute/kernels/scalar_cast_test.cc b/cpp/src/arrow/compute/kernels/scalar_cast_test.cc index 864ec4afce0c..4f9ce188b81a 100644 --- a/cpp/src/arrow/compute/kernels/scalar_cast_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_cast_test.cc @@ -1726,6 +1726,34 @@ TEST(Cast, Decimal128ToDecimal32) { } } +// GH-53288: narrowing to Decimal32 must rescale and range-check the full-width +// coefficient. Truncating to 32 bits first silently wrapped the value. +TEST(Cast, Decimal128ToDecimal32NarrowingChecksFullCoefficient) { + CastOptions options; + options.allow_decimal_truncate = false; + + // Ten-digit coefficient does not fit decimal32(9, 0). Truncating first yielded + // 5000000000 - 2^32 = 705032704, which does fit, so the cast wrongly succeeded. + auto too_wide = ArrayFromJSON(decimal128(12, 0), R"(["5000000000."])"); + options.to_type = decimal32(9, 0); + CheckCastFails(too_wide, options); + + // 536870912000 is exactly 125 * 2^32, so the low 32 bits are zero. Rescaling by + // -3 first gives 536870912, which fits decimal32(9, -3) and must be preserved. + Decimal32Builder builder(decimal32(9, -3)); + ASSERT_OK(builder.Append(Decimal32(536870912))); // 536870912 * 10^3 + ASSERT_OK_AND_ASSIGN(auto expected_negative_scale, builder.Finish()); + + auto negative_scale_input = ArrayFromJSON(decimal128(12, 0), R"(["536870912000."])"); + CheckCast(negative_scale_input, expected_negative_scale, options); + + // 30000000.00 has an unscaled value of 3000000000, which wraps to a negative + // int32. The wrapped value is no longer divisible by 100, so the exact rescale + // was rejected as a data loss. + auto exact_rescale = ArrayFromJSON(decimal128(12, 2), R"(["30000000.00"])"); + CheckCast(exact_rescale, ArrayFromJSON(decimal32(9, 0), R"(["30000000."])"), options); +} + TEST(Cast, Decimal128ToDecimal64) { CastOptions options; From 75bd205fa99068ee338581d4749a5a010f8d7e56 Mon Sep 17 00:00:00 2001 From: Soki Fukuda Date: Tue, 13 Oct 2026 03:23:43 +0900 Subject: [PATCH 2/2] GH-53288: [C++] Cover the other source widths and truncation mode The specialization this PR fixes serves every Decimal{32,64,128,256} -> Decimal32 cast, but the test only exercised Decimal128 as the source. Add decimal64 and decimal256 sources, both of which silently wrapped the same way before the fix. Also pin the allow_decimal_truncate=true behaviour. The rescale now happens at the source width there too, so the downscale of 536870912000 to decimal32(9, -3) sees the full coefficient instead of its low 32 bits. That is a user-visible change and was previously untested. Rename the test accordingly, since it no longer only covers Decimal128. --- cpp/src/arrow/compute/kernels/scalar_cast_test.cc | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_cast_test.cc b/cpp/src/arrow/compute/kernels/scalar_cast_test.cc index 4f9ce188b81a..56999295d792 100644 --- a/cpp/src/arrow/compute/kernels/scalar_cast_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_cast_test.cc @@ -1727,8 +1727,9 @@ TEST(Cast, Decimal128ToDecimal32) { } // GH-53288: narrowing to Decimal32 must rescale and range-check the full-width -// coefficient. Truncating to 32 bits first silently wrapped the value. -TEST(Cast, Decimal128ToDecimal32NarrowingChecksFullCoefficient) { +// coefficient. Truncating to 32 bits first silently wrapped the value. One +// specialization serves every source width, so all of them were affected. +TEST(Cast, DecimalToDecimal32NarrowingChecksFullCoefficient) { CastOptions options; options.allow_decimal_truncate = false; @@ -1752,6 +1753,16 @@ TEST(Cast, Decimal128ToDecimal32NarrowingChecksFullCoefficient) { // was rejected as a data loss. auto exact_rescale = ArrayFromJSON(decimal128(12, 2), R"(["30000000.00"])"); CheckCast(exact_rescale, ArrayFromJSON(decimal32(9, 0), R"(["30000000."])"), options); + + // The same specialization serves every source width, so check the other two. + options.to_type = decimal32(9, 0); + CheckCastFails(ArrayFromJSON(decimal64(18, 0), R"(["5000000000."])"), options); + CheckCastFails(ArrayFromJSON(decimal256(40, 0), R"(["5000000000."])"), options); + + // With truncation allowed the rescale now also happens at the source width, so + // the downscale sees 536870912000 rather than its low 32 bits. + options.allow_decimal_truncate = true; + CheckCast(negative_scale_input, expected_negative_scale, options); } TEST(Cast, Decimal128ToDecimal64) {