createcentury commented on issue #53288:
URL: https://github.com/apache/arrow/issues/53288#issuecomment-6129107576
I had a look at this and the root cause is an ordering inversion in
`DecimalConversions<Decimal32, InDecimal>`.
The helper in `scalar_cast_numeric.cc` follows a convention its own comments
spell out: widening conversions "convert then scale", narrowing ones "scale
then truncate". `SafeRescaleDecimal` depends on it — it calls `ConvertInput` →
`Rescale` → `FitsInPrecision` → `ConvertOutput`, so on every other narrowing
path the overflow check sees the full-width coefficient.
`DecimalConversions<Decimal64, Decimal128>` is the reference shape.
The Decimal32 specialization is the only one that narrows in `ConvertInput`:
```cpp
static Decimal32 ConvertInput(InDecimal&& val) { return
Decimal32(val.low_bits()); }
```
so both `Rescale` and `FitsInPrecision` run against an already-truncated
coefficient — and this is the single implementation behind every
`Decimal{32,64,128,256} -> Decimal32` cast.
It accounts for the three reported cases exactly:
- `5000000000 - 2^32 = 705032704`
- `536870912000 = 125 * 2^32`, so the low 32 bits are `0`, which then
rescales to `0E+3`
- `3000000000 - 2^32 = -1294967296`, which is no longer divisible by 100,
hence the spurious "would cause data loss"
The fix is to give the specialization the same shape as the Decimal64 one:
keep `InDecimal` through `ConvertInput` and narrow in `ConvertOutput`.
`low_bits()` is defined on `SmallBasicDecimal` as well as on
`BasicDecimal128`/`BasicDecimal256`, so a single generic specialization still
covers all four source types.
I'm putting together a PR with regression tests for the three cases.
@vera-rubin, let me know if you were already working on one.
--
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]
