Jefffrey commented on code in PR #10996:
URL: https://github.com/apache/arrow-rs/pull/10996#discussion_r4043054410
##########
arrow-arith/src/numeric.rs:
##########
@@ -1140,12 +1149,14 @@ fn decimal_op<T: DecimalType>(
// max(s1, s2)
let result_scale = *s1.max(s2);
// min(p1-s1, p2 -s2) + max( s1,s2 )
- let result_precision =
- (result_scale.saturating_add((*p1 as i8 - s1).min(*p2 as i8 -
s2)) as u8)
- .min(T::MAX_PRECISION);
+ let result_precision = (result_scale as i16
+ + (*p1 as i16 - *s1 as i16).min(*p2 as i16 - *s2 as i16))
+ .min(T::MAX_PRECISION as i16) as u8;
- let l_mul = T::Native::usize_as(10).pow_wrapping((result_scale -
s1) as _);
- let r_mul = T::Native::usize_as(10).pow_wrapping((result_scale -
s2) as _);
+ let l_mul =
+ T::Native::usize_as(10).pow_checked((result_scale as i16 - *s1
as i16) as _)?;
+ let r_mul =
+ T::Native::usize_as(10).pow_checked((result_scale as i16 - *s2
as i16) as _)?;
Review Comment:
similarly here
##########
arrow-arith/src/numeric.rs:
##########
@@ -1534,6 +1545,82 @@ mod tests {
assert_eq!(err, "Divide by zero error");
}
+ #[test]
+ fn test_decimal256_negative_scale_metadata() {
+ for (precision, scale, expected_precision) in
+ [(76, -51, 76), (76, -52, 76), (76, -76, 76), (20, -128, 21)]
+ {
+ let a = Decimal256Array::from(vec![Some(i256::ONE),
Some(i256::MINUS_ONE), None])
+ .with_precision_and_scale(precision, scale)
+ .unwrap();
+ let b = Decimal256Array::from(vec![i256::from_i128(2), i256::ONE,
i256::ONE])
+ .with_precision_and_scale(precision, scale)
+ .unwrap();
+ for (operation, values, precision) in [
+ (
+ add as fn(_, _) -> _,
+ [Some(3), Some(0), None],
+ expected_precision,
+ ),
+ (add_wrapping, [Some(3), Some(0), None], expected_precision),
+ (sub, [Some(-1), Some(-2), None], expected_precision),
+ (sub_wrapping, [Some(-1), Some(-2), None], expected_precision),
+ (rem, [Some(1), Some(0), None], precision),
+ ] {
+ let expected =
+ Decimal256Array::from(values.map(|x|
x.map(i256::from_i128)).to_vec())
+ .with_precision_and_scale(precision, scale)
Review Comment:
theres too many parameters going on here; cant we split the tests up. for
example, grouping in `rem` is confusing since its expected precision is
differen from the others, and the add/sub wrapping variations will have same
result anyway
##########
arrow-arith/src/numeric.rs:
##########
@@ -1047,12 +1047,13 @@ fn decimal_op<T: DecimalType>(
// max(s1, s2) + max(p1-s1, p2-s2) + 1
let result_precision =
- (result_scale.saturating_add((*p1 as i8 - s1).max(*p2 as i8 -
s2)) as u8)
- .saturating_add(1)
- .min(T::MAX_PRECISION);
+ (result_scale as i16 + (*p1 as i16 - *s1 as i16).max(*p2 as
i16 - *s2 as i16) + 1)
Review Comment:
can we refactor these to not have the `as i16` everywhere? its a bit hard to
read
for example we could make `result_scale` an `i16` above, then we dont need
to constantly cast it and its only used once more down below in
`with_precision_and_scale` so thats just a single `as i8`
##########
arrow-arith/src/numeric.rs:
##########
@@ -1534,6 +1545,82 @@ mod tests {
assert_eq!(err, "Divide by zero error");
}
+ #[test]
+ fn test_decimal256_negative_scale_metadata() {
+ for (precision, scale, expected_precision) in
+ [(76, -51, 76), (76, -52, 76), (76, -76, 76), (20, -128, 21)]
+ {
+ let a = Decimal256Array::from(vec![Some(i256::ONE),
Some(i256::MINUS_ONE), None])
+ .with_precision_and_scale(precision, scale)
+ .unwrap();
+ let b = Decimal256Array::from(vec![i256::from_i128(2), i256::ONE,
i256::ONE])
+ .with_precision_and_scale(precision, scale)
+ .unwrap();
+ for (operation, values, precision) in [
+ (
+ add as fn(_, _) -> _,
+ [Some(3), Some(0), None],
+ expected_precision,
+ ),
+ (add_wrapping, [Some(3), Some(0), None], expected_precision),
+ (sub, [Some(-1), Some(-2), None], expected_precision),
+ (sub_wrapping, [Some(-1), Some(-2), None], expected_precision),
+ (rem, [Some(1), Some(0), None], precision),
+ ] {
+ let expected =
+ Decimal256Array::from(values.map(|x|
x.map(i256::from_i128)).to_vec())
+ .with_precision_and_scale(precision, scale)
+ .unwrap();
+ let result = operation(&a, &b).unwrap();
+ assert_eq!(result.as_primitive::<Decimal256Type>(), &expected);
+ }
+ }
+ }
+
+ #[test]
+ fn test_decimal256_extreme_scale_difference() {
+ let a = Decimal256Array::from(vec![i256::ONE])
+ .with_precision_and_scale(20, i8::MIN)
+ .unwrap();
+ let b = Decimal256Array::from(vec![i256::from_i128(2)])
+ .with_precision_and_scale(20, i8::MIN + 1)
+ .unwrap();
+ for (operation, value, precision) in
+ [(add as fn(_, _) -> _, 12, 22), (sub, 8, 22), (rem, 0, 20)]
+ {
+ let result = operation(&a, &b).unwrap();
+ assert_eq!(result.data_type(), &DataType::Decimal256(precision,
-127));
+ assert_eq!(
+ result.as_primitive::<Decimal256Type>().value(0),
+ i256::from_i128(value)
+ );
+ }
+ let b = b.with_precision_and_scale(76, 76).unwrap();
+ for operation in [add, sub, rem] {
+ assert!(matches!(
+ operation(&a, &b),
+ Err(ArrowError::ArithmeticOverflow(_))
+ ));
+ }
+ }
+
+ #[test]
+ fn test_decimal256_multiply_minimum_scale() {
+ let a = Decimal256Array::from(vec![i256::ONE])
+ .with_precision_and_scale(76, -64)
+ .unwrap();
+ for operation in [mul, mul_wrapping] {
+ let result = operation(&a, &a).unwrap();
+ assert_eq!(result.data_type(), &DataType::Decimal256(76, i8::MIN));
+ assert_eq!(result.as_primitive::<Decimal256Type>().value(0),
i256::ONE);
+ let b = a.clone().with_precision_and_scale(76, -65).unwrap();
+ assert!(matches!(
+ operation(&a, &b),
+ Err(ArrowError::InvalidArgumentError(_))
Review Comment:
same here
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -11023,6 +11023,82 @@ mod tests {
assert!(result.value(0) < 0.0); // Negative result
}
+ #[test]
+ fn test_cast_decimal256_negative_scale_metadata() {
+ for scale in [-51, -52, -75, -76] {
+ let input = Decimal256Array::from(vec![
+ Some(i256::ZERO),
+ Some(i256::ONE),
+ Some(i256::MINUS_ONE),
+ Some(i256::from_i128(6)),
+ None,
+ ])
+ .with_precision_and_scale(76, scale)
+ .unwrap();
+ let value = i256::from_i128(10).pow_wrapping((-scale) as u32);
+ let values = if scale == -76 {
+ vec![Some(i256::ZERO), None, None, None, None]
+ } else {
+ vec![
+ Some(i256::ZERO),
+ Some(value),
+ Some(-value),
+ Some(value * i256::from_i128(6)),
+ None,
+ ]
+ };
+ let expected = Decimal256Array::from(values)
+ .with_precision_and_scale(76, 0)
+ .unwrap();
+ let result = cast(&input, &DataType::Decimal256(76, 0)).unwrap();
+ assert_eq!(result.as_primitive::<Decimal256Type>(), &expected);
+ let result = cast_with_options(
+ &input,
+ &DataType::Decimal256(76, 0),
+ &CastOptions {
+ safe: false,
+ ..Default::default()
+ },
+ );
+ if scale == -76 {
+ assert!(result.is_err());
+ } else {
+ assert_eq!(result.unwrap().as_primitive::<Decimal256Type>(),
&expected);
Review Comment:
i dont like having this if condition just because its an exceptional case
being lumped in with the others; can we unroll the loop instead, and separate
exceptional cases from others
##########
arrow-arith/src/numeric.rs:
##########
@@ -1534,6 +1545,82 @@ mod tests {
assert_eq!(err, "Divide by zero error");
}
+ #[test]
+ fn test_decimal256_negative_scale_metadata() {
+ for (precision, scale, expected_precision) in
+ [(76, -51, 76), (76, -52, 76), (76, -76, 76), (20, -128, 21)]
+ {
+ let a = Decimal256Array::from(vec![Some(i256::ONE),
Some(i256::MINUS_ONE), None])
+ .with_precision_and_scale(precision, scale)
+ .unwrap();
+ let b = Decimal256Array::from(vec![i256::from_i128(2), i256::ONE,
i256::ONE])
+ .with_precision_and_scale(precision, scale)
+ .unwrap();
+ for (operation, values, precision) in [
+ (
+ add as fn(_, _) -> _,
+ [Some(3), Some(0), None],
+ expected_precision,
+ ),
+ (add_wrapping, [Some(3), Some(0), None], expected_precision),
+ (sub, [Some(-1), Some(-2), None], expected_precision),
+ (sub_wrapping, [Some(-1), Some(-2), None], expected_precision),
+ (rem, [Some(1), Some(0), None], precision),
+ ] {
+ let expected =
+ Decimal256Array::from(values.map(|x|
x.map(i256::from_i128)).to_vec())
+ .with_precision_and_scale(precision, scale)
+ .unwrap();
+ let result = operation(&a, &b).unwrap();
+ assert_eq!(result.as_primitive::<Decimal256Type>(), &expected);
+ }
+ }
+ }
+
+ #[test]
+ fn test_decimal256_extreme_scale_difference() {
+ let a = Decimal256Array::from(vec![i256::ONE])
+ .with_precision_and_scale(20, i8::MIN)
+ .unwrap();
+ let b = Decimal256Array::from(vec![i256::from_i128(2)])
+ .with_precision_and_scale(20, i8::MIN + 1)
+ .unwrap();
+ for (operation, value, precision) in
+ [(add as fn(_, _) -> _, 12, 22), (sub, 8, 22), (rem, 0, 20)]
+ {
+ let result = operation(&a, &b).unwrap();
+ assert_eq!(result.data_type(), &DataType::Decimal256(precision,
-127));
+ assert_eq!(
+ result.as_primitive::<Decimal256Type>().value(0),
+ i256::from_i128(value)
+ );
+ }
+ let b = b.with_precision_and_scale(76, 76).unwrap();
+ for operation in [add, sub, rem] {
+ assert!(matches!(
+ operation(&a, &b),
+ Err(ArrowError::ArithmeticOverflow(_))
Review Comment:
can we assert the error message to ensure we're hitting the expected one
##########
arrow-arith/src/numeric.rs:
##########
@@ -1534,6 +1545,82 @@ mod tests {
assert_eq!(err, "Divide by zero error");
}
+ #[test]
+ fn test_decimal256_negative_scale_metadata() {
+ for (precision, scale, expected_precision) in
+ [(76, -51, 76), (76, -52, 76), (76, -76, 76), (20, -128, 21)]
+ {
+ let a = Decimal256Array::from(vec![Some(i256::ONE),
Some(i256::MINUS_ONE), None])
+ .with_precision_and_scale(precision, scale)
+ .unwrap();
+ let b = Decimal256Array::from(vec![i256::from_i128(2), i256::ONE,
i256::ONE])
+ .with_precision_and_scale(precision, scale)
+ .unwrap();
+ for (operation, values, precision) in [
+ (
+ add as fn(_, _) -> _,
+ [Some(3), Some(0), None],
+ expected_precision,
+ ),
+ (add_wrapping, [Some(3), Some(0), None], expected_precision),
+ (sub, [Some(-1), Some(-2), None], expected_precision),
+ (sub_wrapping, [Some(-1), Some(-2), None], expected_precision),
+ (rem, [Some(1), Some(0), None], precision),
+ ] {
+ let expected =
+ Decimal256Array::from(values.map(|x|
x.map(i256::from_i128)).to_vec())
+ .with_precision_and_scale(precision, scale)
+ .unwrap();
+ let result = operation(&a, &b).unwrap();
+ assert_eq!(result.as_primitive::<Decimal256Type>(), &expected);
+ }
+ }
+ }
+
+ #[test]
+ fn test_decimal256_extreme_scale_difference() {
+ let a = Decimal256Array::from(vec![i256::ONE])
+ .with_precision_and_scale(20, i8::MIN)
+ .unwrap();
+ let b = Decimal256Array::from(vec![i256::from_i128(2)])
+ .with_precision_and_scale(20, i8::MIN + 1)
+ .unwrap();
+ for (operation, value, precision) in
+ [(add as fn(_, _) -> _, 12, 22), (sub, 8, 22), (rem, 0, 20)]
+ {
+ let result = operation(&a, &b).unwrap();
+ assert_eq!(result.data_type(), &DataType::Decimal256(precision,
-127));
Review Comment:
similarly i feel its better to unwrap the loop as it feels like adding
complexity with tracking these parameters, not to mention the closure for the
operation
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -11023,6 +11023,82 @@ mod tests {
assert!(result.value(0) < 0.0); // Negative result
}
+ #[test]
+ fn test_cast_decimal256_negative_scale_metadata() {
+ for scale in [-51, -52, -75, -76] {
+ let input = Decimal256Array::from(vec![
+ Some(i256::ZERO),
+ Some(i256::ONE),
+ Some(i256::MINUS_ONE),
+ Some(i256::from_i128(6)),
+ None,
+ ])
+ .with_precision_and_scale(76, scale)
+ .unwrap();
+ let value = i256::from_i128(10).pow_wrapping((-scale) as u32);
+ let values = if scale == -76 {
+ vec![Some(i256::ZERO), None, None, None, None]
+ } else {
+ vec![
+ Some(i256::ZERO),
+ Some(value),
+ Some(-value),
+ Some(value * i256::from_i128(6)),
+ None,
+ ]
+ };
+ let expected = Decimal256Array::from(values)
+ .with_precision_and_scale(76, 0)
+ .unwrap();
+ let result = cast(&input, &DataType::Decimal256(76, 0)).unwrap();
+ assert_eq!(result.as_primitive::<Decimal256Type>(), &expected);
+ let result = cast_with_options(
+ &input,
+ &DataType::Decimal256(76, 0),
+ &CastOptions {
+ safe: false,
+ ..Default::default()
+ },
+ );
+ if scale == -76 {
+ assert!(result.is_err());
+ } else {
+ assert_eq!(result.unwrap().as_primitive::<Decimal256Type>(),
&expected);
+ }
+ }
+ }
+
+ #[test]
+ fn test_cast_decimal256_extreme_scale_difference() {
+ for (low_scale, high_scale) in [(-76, 76), (i8::MIN, 76)] {
+ let input = Decimal256Array::from(vec![Some(i256::ONE),
Some(i256::MINUS_ONE), None])
+ .with_precision_and_scale(76, low_scale)
+ .unwrap();
+ for safe in [true, false] {
+ let options = CastOptions {
+ safe,
+ ..Default::default()
+ };
+ assert!(
+ cast_with_options(&input, &DataType::Decimal256(76,
high_scale), &options)
+ .is_err()
Review Comment:
can we assert the error message to ensure its the expected one?
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -11023,6 +11023,82 @@ mod tests {
assert!(result.value(0) < 0.0); // Negative result
}
+ #[test]
+ fn test_cast_decimal256_negative_scale_metadata() {
+ for scale in [-51, -52, -75, -76] {
+ let input = Decimal256Array::from(vec![
+ Some(i256::ZERO),
+ Some(i256::ONE),
+ Some(i256::MINUS_ONE),
+ Some(i256::from_i128(6)),
+ None,
+ ])
+ .with_precision_and_scale(76, scale)
+ .unwrap();
+ let value = i256::from_i128(10).pow_wrapping((-scale) as u32);
+ let values = if scale == -76 {
+ vec![Some(i256::ZERO), None, None, None, None]
+ } else {
+ vec![
+ Some(i256::ZERO),
+ Some(value),
+ Some(-value),
+ Some(value * i256::from_i128(6)),
+ None,
+ ]
+ };
+ let expected = Decimal256Array::from(values)
+ .with_precision_and_scale(76, 0)
+ .unwrap();
+ let result = cast(&input, &DataType::Decimal256(76, 0)).unwrap();
+ assert_eq!(result.as_primitive::<Decimal256Type>(), &expected);
+ let result = cast_with_options(
+ &input,
+ &DataType::Decimal256(76, 0),
+ &CastOptions {
+ safe: false,
+ ..Default::default()
+ },
+ );
+ if scale == -76 {
+ assert!(result.is_err());
+ } else {
+ assert_eq!(result.unwrap().as_primitive::<Decimal256Type>(),
&expected);
+ }
+ }
+ }
+
+ #[test]
+ fn test_cast_decimal256_extreme_scale_difference() {
+ for (low_scale, high_scale) in [(-76, 76), (i8::MIN, 76)] {
Review Comment:
is there really a need to have both these test cases? wouldnt `i8::MIN`
cover them both?
--
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]
