createcentury opened a new pull request, #53327:
URL: https://github.com/apache/arrow/pull/53327
### 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.
### Are these changes tested?
Yes. `Cast.Decimal128ToDecimal32NarrowingChecksFullCoefficient` covers the
three reported
cases. I verified it fails on the unpatched kernel — the first case wrongly
succeeds and
the other two produce the wrong value or a spurious error — and passes with
the change.
The full `arrow-compute-scalar-cast-test` suite passes, 109 tests, including
the existing
Decimal tests that exercise both `allow_decimal_truncate` settings.
### 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](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code),
please disclose below whether and how AI was used in this PR.
**PR code and description written by:**
- [ ] Human
- [x] AI
**Reviewed before submission by:**
- [x] Human
- [ ] AI
- [ ] Not reviewed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
