Repository navigation
GH-53288: [C++] Rescale and range-check before narrowing decimals to Decimal32 - #53327
Open
createcentury wants to merge 2 commits into
Open
createcentury wants to merge 2 commits into
createcentury wants to merge 2 commits into
Conversation
…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.
|
|
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
DecimalConversionsinscalar_cast_numeric.ccfollows a convention its own commentsstate: widening conversions "convert then scale", narrowing ones "scale then truncate".
SafeRescaleDecimalrelies on it, callingConvertInput→Rescale→FitsInPrecision→
ConvertOutput, so on every narrowing path the overflow check sees the full-widthcoefficient.
DecimalConversions<Decimal32, InDecimal>is the only specialization that narrows inConvertInput, and it is the single implementation behind everyDecimal{32,64,128,256} -> Decimal32cast. BothRescaleandFitsInPrecisiontherefore ran against an already-truncated coefficient.
This accounts for the three cases in #53288 exactly:
5000000000 -> decimal32(9, 0)returned705032704, which is5000000000 - 2^32536870912000 -> decimal32(9, -3)returned0E+3, because536870912000is exactly125 * 2^32and the low 32 bits are zero30000000.00 -> decimal32(9, 0)was rejected, because the unscaled value3000000000wraps to
-1294967296, which is no longer divisible by 100What changes are included in this PR?
DecimalConversions<Decimal32, InDecimal>now has the same shape asDecimalConversions<Decimal64, Decimal128>:ConvertInputkeepsInDecimalandConvertOutputnarrows.low_bits()is defined onSmallBasicDecimalas well as onBasicDecimal128/BasicDecimal256, so one generic specialization still covers all foursource types.
The unsafe paths (
allow_decimal_truncate=true) change order too, which makes Decimal32consistent with the Decimal64 and Decimal128 outputs.
I audited all ten
DecimalConversionsspecializations:Decimal32was the only onewhose
ConvertInputreturned the narrow output type, so this is the only place theinversion occurred.
Are these changes tested?
Yes.
Cast.DecimalToDecimal32NarrowingChecksFullCoefficientcovers the three reportedcases. I verified it fails on the unpatched kernel and passes with the change. On the
unpatched kernel the three produce, respectively:
Since one specialization serves every source width, the test also covers
decimal64anddecimal256sources, both of which wrapped the same way before the fix. It additionallypins the
allow_decimal_truncate=truebehaviour, which this PR changes and which waspreviously untested.
The full
arrow-compute-scalar-cast-testsuite passes, 109 tests, including the existingDecimal tests that exercise both
allow_decimal_truncatesettings.DecimalConversionsis used only within
scalar_cast_numeric.cc, so the blast radius is the decimal castkernels.
Are there any user-facing changes?
Yes.
casttodecimal32now rejects values whose coefficient does not fit the targetprecision 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:
5000000000became705032704with 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:
Reviewed before submission by: