You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "scovich (via GitHub)" <gi...@apache.org> on 2025/10/09 19:42:14 UTC
[PR] Fix broken decimal->decimal casting with large scale reduction [arrow-rs]
scovich opened a new pull request, #8580:
URL: https://github.com/apache/arrow-rs/pull/8580
# Which issue does this PR close?
- Closes https://github.com/apache/arrow-rs/issues/8579
# 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_checked` call with a lookup into the (already existing) `MAX_DECIMALXX_FOR_EACH_PRECISION` array. This requires adding a new `MAX_FOR_EACH_PRECISION` constant to the `DecimalType` trait, 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 `DecimalType` trait.
A class of decimal conversions that used to fail will now (correctly) produce zeros instead.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix broken decimal->decimal casting with large scale reduction [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8580:
URL: https://github.com/apache/arrow-rs/pull/8580#discussion_r2417932460
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -3084,6 +3084,32 @@ mod tests {
);
}
+ #[test]
+ fn test_cast_decimal32_to_decimal32_large_scale_reduction() {
Review 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:
```
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -3106,6 +3132,37 @@ mod tests {
);
}
+ #[test]
+ fn test_cast_decimal64_to_decimal64_large_scale_reduction() {
Review Comment:
i agree this seems adequate
##########
arrow-array/src/arithmetic.rs:
##########
@@ -288,7 +288,7 @@ native_type_op!(u8);
native_type_op!(u16);
native_type_op!(u32);
native_type_op!(u64);
-native_type_op!(i256, i256::ZERO, i256::ONE, i256::MIN, i256::MAX);
Review Comment:
I verified that this form of the macro simply re-calls the same macro with the same arguments 👍
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix broken decimal->decimal casting with large scale reduction [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8580:
URL: https://github.com/apache/arrow-rs/pull/8580#issuecomment-3387264026
CC @alamb -- not sure who the best reviewer might be?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix broken decimal->decimal casting with large scale reduction [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8580:
URL: https://github.com/apache/arrow-rs/pull/8580#discussion_r2417792272
##########
arrow-array/src/arithmetic.rs:
##########
@@ -288,7 +288,7 @@ native_type_op!(u8);
native_type_op!(u16);
native_type_op!(u32);
native_type_op!(u64);
-native_type_op!(i256, i256::ZERO, i256::ONE, i256::MIN, i256::MAX);
Review Comment:
opportunistic cleanup
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -188,11 +188,19 @@ where
// [99999] -> [99] + 1 = [100], a cast to Decimal(2, 0) would not be possible
let is_infallible_cast = (input_precision as i8) - delta_scale < (output_precision as i8);
- let div = I::Native::from_decimal(10_i128)
- .unwrap()
- .pow_checked(delta_scale as u32)?;
+ // delta_scale is guaranteed to be > 0, but may also be larger than I::MAX_PRECISION. If so, the
+ // scale change divides out more digits than the input has precision and the result of the cast
+ // is always zero. For example, if we try to apply delta_scale=10 a decimal32 value, the largest
+ // possible result is 999999999/10000000000 = 0.0999999999, which rounds to zero. Smaller values
+ // (e.g. 1/10000000000) or larger delta_scale (e.g. 999999999/10000000000000) produce even
+ // smaller results, which also round to zero. In that case, just return an array of zeros.
+ let Some(max) = I::MAX_FOR_EACH_PRECISION.get(delta_scale as usize) else {
+ let zeros = vec![O::Native::ZERO; array.len()];
+ return Ok(PrimitiveArray::new(zeros.into(), array.nulls().cloned()));
+ };
- let half = div.div_wrapping(I::Native::from_usize(2).unwrap());
+ let div = max.add_wrapping(I::Native::ONE);
Review Comment:
Rather than paying an exponentiation, just look up the max value for that precision and add one.
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -188,11 +188,19 @@ where
// [99999] -> [99] + 1 = [100], a cast to Decimal(2, 0) would not be possible
let is_infallible_cast = (input_precision as i8) - delta_scale < (output_precision as i8);
- let div = I::Native::from_decimal(10_i128)
- .unwrap()
- .pow_checked(delta_scale as u32)?;
+ // delta_scale is guaranteed to be > 0, but may also be larger than I::MAX_PRECISION. If so, the
+ // scale change divides out more digits than the input has precision and the result of the cast
+ // is always zero. For example, if we try to apply delta_scale=10 a decimal32 value, the largest
+ // possible result is 999999999/10000000000 = 0.0999999999, which rounds to zero. Smaller values
+ // (e.g. 1/10000000000) or larger delta_scale (e.g. 999999999/10000000000000) produce even
+ // smaller results, which also round to zero. In that case, just return an array of zeros.
+ let Some(max) = I::MAX_FOR_EACH_PRECISION.get(delta_scale as usize) else {
+ let zeros = vec![O::Native::ZERO; array.len()];
+ return Ok(PrimitiveArray::new(zeros.into(), array.nulls().cloned()));
+ };
- 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));
Review 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.
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -3106,6 +3132,37 @@ mod tests {
);
}
+ #[test]
+ fn test_cast_decimal64_to_decimal64_large_scale_reduction() {
Review 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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix broken decimal->decimal casting with large scale reduction [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8580:
URL: https://github.com/apache/arrow-rs/pull/8580
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org