Fix broken decimal->decimal casting with large scale reduction #8580
Conversation
|
CC @alamb -- not sure who the best reviewer might be? |
| native_type_op!(u16); | ||
| native_type_op!(u32); | ||
| native_type_op!(u64); | ||
| native_type_op!(i256, i256::ZERO, i256::ONE, i256::MIN, i256::MAX); |
There was a problem hiding this comment.
opportunistic cleanup
There was a problem hiding this comment.
I verified that this form of the macro simply re-calls the same macro with the same arguments 👍
| }; | ||
|
|
||
| let half = div.div_wrapping(I::Native::from_usize(2).unwrap()); | ||
| let div = max.add_wrapping(I::Native::ONE); |
There was a problem hiding this comment.
Rather than paying an exponentiation, just look up the max value for that precision and add one.
|
|
||
| let half = div.div_wrapping(I::Native::from_usize(2).unwrap()); | ||
| let div = max.add_wrapping(I::Native::ONE); | ||
| let half = div.div_wrapping(I::Native::ONE.add_wrapping(I::Native::ONE)); |
There was a problem hiding this comment.
Opportunistic cleanup: compute 2 as 1+1 (infallible) instead of converting from 2_usize (needs unwrap). It's fairly likely that the compiler emits the same code either way, tho, thanks to aggressive inlining.
| } | ||
|
|
||
| #[test] | ||
| fn test_cast_decimal64_to_decimal64_large_scale_reduction() { |
There was a problem hiding this comment.
It's not obvious to me that we need this second version of the test, given that it's all generic code anyway.
I intentionally avoided adding cases for 128- and 256-bit decimals because IMO they add no value -- any problems in the constants should be caught by other tests, and two data points should suffice to confirm that the new code doesn't hide any size-specific assumptions.
| native_type_op!(u16); | ||
| native_type_op!(u32); | ||
| native_type_op!(u64); | ||
| native_type_op!(i256, i256::ZERO, i256::ONE, i256::MIN, i256::MAX); |
There was a problem hiding this comment.
I verified that this form of the macro simply re-calls the same macro with the same arguments 👍
| } | ||
|
|
||
| #[test] | ||
| fn test_cast_decimal32_to_decimal32_large_scale_reduction() { |
There was a problem hiding this comment.
I verified this test fails without the code in this PR:
called `Result::unwrap()` on an `Err` value: ArithmeticOverflow("Overflow happened on: 10 ^ 10")
thread 'cast::tests::test_cast_decimal32_to_decimal32_large_scale_reduction' panicked at arrow-cast/src/cast/mod.rs:3105:9:
called `Result::unwrap()` on an `Err` value: ArithmeticOverflow("Overflow happened on: 10 ^ 10")
stack backtrace:
| } | ||
|
|
||
| #[test] | ||
| fn test_cast_decimal64_to_decimal64_large_scale_reduction() { |
3 participants
Which issue does this PR close?
Rationale for this change
Bug fix
What changes are included in this PR?
Detect and directly handle large scale reductions, instead of failing on accident because the computed divisor overflows.
Also, replace the
pow_checkedcall with a lookup into the (already existing)MAX_DECIMALXX_FOR_EACH_PRECISIONarray. This requires adding a newMAX_FOR_EACH_PRECISIONconstant to theDecimalTypetrait, but the corresponding arrays were already public so this seems ok?Are these changes tested?
New unit tests exercise the scenario (and its boundary case). The tests fail without this fix.
Are there any user-facing changes?
New constant on the public
DecimalTypetrait.A class of decimal conversions that used to fail will now (correctly) produce zeros instead.