Skip to content

GH-53288: [C++] Rescale and range-check before narrowing decimals to Decimal32 - #53327

Open
createcentury wants to merge 2 commits into
apache:mainfrom
createcentury:fix-53288-decimal32-narrowing
Open

createcentury wants to merge 2 commits into
apache:mainfrom
createcentury:fix-53288-decimal32-narrowing

Conversation

@createcentury

@createcentury createcentury commented Oct 12, 2026 •

Copy link
Copy Markdown

Rationale for this change

DecimalConversions in scalar_cast_numeric.cc 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<Decimal32, InDecimal> is the only specialization that narrows in
ConvertInput, and it is the single implementation behind every
Decimal{32,64,128,256} -> Decimal32 cast. Both Rescale and FitsInPrecision
therefore ran against an already-truncated coefficient.

This accounts for the three cases in #53288 exactly:

  • 5000000000 -> decimal32(9, 0) returned 705032704, which is 5000000000 - 2^32
  • 536870912000 -> decimal32(9, -3) returned 0E+3, because 536870912000 is exactly
    125 * 2^32 and the low 32 bits are zero
  • 30000000.00 -> decimal32(9, 0) was rejected, because the unscaled value 3000000000
    wraps to -1294967296, which is no longer divisible by 100

What changes are included in this PR?

DecimalConversions<Decimal32, InDecimal> now has the same shape as
DecimalConversions<Decimal64, Decimal128>: 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.

The unsafe paths (allow_decimal_truncate=true) change order too, which makes Decimal32
consistent with the Decimal64 and Decimal128 outputs.

I audited all ten DecimalConversions specializations: Decimal32 was the only one
whose ConvertInput returned the narrow output type, so this is the only place the
inversion occurred.

Are these changes tested?

Yes. Cast.DecimalToDecimal32NarrowingChecksFullCoefficient covers the three reported
cases. I verified it fails on the unpatched kernel and passes with the change. On the
unpatched kernel the three produce, respectively:

Expected 'Cast(input, options)' to fail with Invalid, but got OK
  to_type: decimal32(9, 0) / from_type: decimal128(12, 0) / input: [5000000000]
cast(536870912000) = 0E+3 != 5.36870912E+11
'...' failed with Invalid: Rescaling Decimal value would cause data loss

Since one specialization serves every source width, the test also covers decimal64 and
decimal256 sources, both of which wrapped the same way before the fix. It additionally
pins the allow_decimal_truncate=true behaviour, which this PR changes and which was
previously untested.

The full arrow-compute-scalar-cast-test suite passes, 109 tests, including the existing
Decimal tests that exercise both allow_decimal_truncate settings. DecimalConversions
is used only within scalar_cast_numeric.cc, so the blast radius is the decimal cast
kernels.

Are there any user-facing changes?

Yes. cast to decimal32 now rejects values whose coefficient does not fit the target
precision instead of silently wrapping, and accepts scale changes that are exact in the
source width. Code that relied on the wrapped results will now see an error.

This PR contains a "Critical Fix". A safe cast produced incorrect data without
raising: 5000000000 became 705032704 with overflow checking enabled.

Was AI used for this PR?

In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.

PR code and description written by:

  • Human
  • AI

Reviewed before submission by:

  • Human
  • AI
  • Not reviewed

…ls 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<Decimal32, InDecimal> 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<Decimal64, Decimal128>:
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.
@github-actions github-actions Bot added the awaiting review Awaiting review label Oct 12, 2026
@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #53288 has been automatically assigned in GitHub to PR creator.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant