Add Datum Arithmetic tests, Fix Interval Substraction (#4480) #4493
Conversation
| @@ -1315,7 +1315,7 @@ mod tests { | |||
| let c = c.as_any().downcast_ref::<Date32Array>().unwrap(); | |||
| assert_eq!( | |||
| c.value(0), | |||
| Date32Type::from_naive_date(NaiveDate::from_ymd_opt(2023, 3, 27).unwrap()) | |||
| Date32Type::from_naive_date(NaiveDate::from_ymd_opt(2023, 3, 28).unwrap()) | |||
There was a problem hiding this comment.
The previous logic was incorrect but happened to give a potentially plausible result. I'm not sure what the correct rounding behaviour is here though
There was a problem hiding this comment.
Date32: https://docs.rs/arrow/latest/arrow/datatypes/enum.IntervalUnit.html
86500 is milliseconds so 86.5 seconds before 2023-03-29
Thus 2023-03-28 seems much more correct to me
tustvold
changed the title
| @@ -1315,7 +1315,7 @@ mod tests { | |||
| let c = c.as_any().downcast_ref::<Date32Array>().unwrap(); | |||
| assert_eq!( | |||
| c.value(0), | |||
| Date32Type::from_naive_date(NaiveDate::from_ymd_opt(2023, 3, 27).unwrap()) | |||
| Date32Type::from_naive_date(NaiveDate::from_ymd_opt(2023, 3, 28).unwrap()) | |||
There was a problem hiding this comment.
Date32: https://docs.rs/arrow/latest/arrow/datatypes/enum.IntervalUnit.html
86500 is milliseconds so 86.5 seconds before 2023-03-29
Thus 2023-03-28 seems much more correct to me
| @@ -89,6 +90,18 @@ enum Op { | |||
| Rem, | |||
| } | |||
|
|
|||
| impl std::fmt::Display for Op { | |||
| fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | |||
| @@ -570,8 +582,8 @@ fn decimal_op<T: DecimalType>( | |||
| .saturating_add(1) | |||
| .min(T::MAX_PRECISION); | |||
|
|
|||
| 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 - s1) as _)?; | |||
There was a problem hiding this comment.
I was a little confused at first about why both AddWrapping and Add using the same function. Then
I realized this is for computing the scale factor
Given MAX_PRECISION is a constant, what is the reason to change this to pow_checked? Maybe it doesn't matter performance wise, I was just curious
There was a problem hiding this comment.
It can overflow on negative scales
| let a = UInt8Array::from(vec![56, 5, 3]); | ||
| let b = UInt8Array::from(vec![200, 2, 5]); | ||
| let err = add(&a, &b).unwrap_err().to_string(); | ||
| assert_eq!(err, "Compute error: Overflow happened on: 56 + 200"); |
| let a = Int16Array::from(vec![21]); | ||
| let b = Int16Array::from(vec![0]); | ||
| let err = div(&a, &b).unwrap_err().to_string(); | ||
| assert_eq!(err, "Divide by zero error"); |
There was a problem hiding this comment.
I didn't see a test for integer remainder %
| .with_precision_and_scale(37, 37) | ||
| .unwrap(); | ||
| let err = mul(&a, &b).unwrap_err().to_string(); | ||
| assert_eq!(err, "Invalid argument error: Output scale of Decimal128(3, 3) * Decimal128(37, 37) would exceed max scale of 38"); |
| let a = PrimitiveArray::<T>::new(values, None); | ||
| let b = IntervalYearMonthArray::from(vec
Which issue does this PR close?
Closes #4480
Closes #4489
Closes #1065
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?