Repository navigation
GH-53288: [C++] Rescale and range-check before narrowing decimals to Decimal32 - #53327
createcentury wants to merge 3 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.
dmitry-chirkov-dremio
left a comment
dmitry-chirkov-dremio
left a comment
There was a problem hiding this comment.
LGTM (changes, test coverage, alignment on the issue etc)
|
@pitrou would you have time to take a look at this one? Short version: The one red check, dmitry-chirkov-dremio approved it on Oct 16. |
| // Scale then truncate. Narrowing in ConvertInput would drop coefficient bits | ||
| // before Rescale and FitsInPrecision could see them. |
There was a problem hiding this comment.
No need to expand on this.
| // Scale then truncate. Narrowing in ConvertInput would drop coefficient bits | |
| // before Rescale and FitsInPrecision could see them. | |
| // Scale then truncate. |
| } | ||
| } | ||
|
|
||
| // GH-53288: narrowing to Decimal32 must rescale and range-check the full-width |
There was a problem hiding this comment.
Please augment Decimal128ToDecimal32 instead
|
@createcentury 1) Please trim down your PR description to make it human-readable; 2) please don't post AI-generated comments. |
54ebfc4 to
835c302
Compare
|
@pitrou thanks🙏
(The workflows are at |
Rationale for this change
DecimalConversions<Decimal32, InDecimal>narrowed the coefficient inConvertInput, soSafeRescaleDecimalrescaled and range-checked analready-wrapped value. Every other narrowing specialization keeps the source
width until
ConvertOutput. Casting5000000000todecimal32(9, 0)returned
705032704with overflow checking enabled.What changes are included in this PR?
The specialization now matches
DecimalConversions<Decimal64, Decimal128>.This reorders the
allow_decimal_truncate=truepath as well.Are these changes tested?
Yes, in
Decimal64ToDecimal32,Decimal128ToDecimal32andDecimal256ToDecimal32. The new cases fail without the kernel change.Are there any user-facing changes?
Casting to
decimal32now rejects coefficients that do not fit the targetprecision instead of wrapping them.
This PR contains a "Critical Fix". A safe cast produced incorrect data
without raising.
Was AI used for this PR?
PR code and description written by:
Reviewed before submission by: