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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
