mbutrovich commented on code in PR #11611:
URL: https://github.com/apache/arrow-rs/pull/11611#discussion_r4299039276
##########
arrow-arith/src/numeric.rs:
##########
@@ -2862,4 +2894,42 @@ mod tests {
test_month_day_nano_ops!(Date32Type, date_to_days);
test_month_day_nano_ops!(Date64Type, date_to_millis);
}
+
+ #[test]
+ fn test_date_sub_interval_min_months() {
+ // Subtracting i32::MIN months requires negating i32::MIN, which
+ // overflows i32: the date kernels must return the Date arithmetic
+ // overflow error instead of panicking in debug builds
+ // https://github.com/apache/arrow-rs/issues/11600
+ let dates = Date32Array::from(vec![0]);
+ let deltas = IntervalYearMonthArray::from(vec![i32::MIN]);
+ let err = sub(&dates, &deltas).unwrap_err();
+ assert!(
+ err.to_string().contains("Date arithmetic overflow"),
+ "unexpected error: {err}"
+ );
+
+ let dates = Date64Array::from(vec![0]);
+ let err = sub(&dates, &deltas).unwrap_err();
+ assert!(
+ err.to_string().contains("Date arithmetic overflow"),
+ "unexpected error: {err}"
+ );
+
+ let deltas =
+
IntervalMonthDayNanoArray::from(vec![IntervalMonthDayNano::new(i32::MIN, 0,
0)]);
+ let dates = Date32Array::from(vec![0]);
+ let err = sub(&dates, &deltas).unwrap_err();
+ assert!(
+ err.to_string().contains("Date arithmetic overflow"),
+ "unexpected error: {err}"
+ );
+
+ let dates = Date64Array::from(vec![0]);
+ let err = sub(&dates, &deltas).unwrap_err();
+ assert!(
+ err.to_string().contains("Date arithmetic overflow"),
+ "unexpected error: {err}"
+ );
Review Comment:
What do you think about asserting the full error message, like the other
kernel overflow tests in this file do
([numeric.rs](https://github.com/apache/arrow-rs/blob/3ef09c668303f46da22a56373bd125a320fecf3b/arrow-arith/src/numeric.rs#L1387-L1395))?
`contains("Date arithmetic overflow")` would also pass if the error came from
a different operand or operation. The full message also checks that the error
reports the operands the caller passed. Looping over the two date arrays keeps
the test short.
```suggestion
let dates: [ArrayRef; 2] = [
Arc::new(Date32Array::from(vec![0])),
Arc::new(Date64Array::from(vec![0])),
];
let year_month = IntervalYearMonthArray::from(vec![i32::MIN]);
let month_day_nano =
IntervalMonthDayNanoArray::from(vec![IntervalMonthDayNano::new(i32::MIN, 0,
0)]);
for dates in &dates {
assert_eq!(
sub(dates, &year_month).unwrap_err().to_string(),
"Compute error: Date arithmetic overflow: 0 - -2147483648
months"
);
assert_eq!(
sub(dates, &month_day_nano).unwrap_err().to_string(),
"Compute error: Date arithmetic overflow: 0 -
IntervalMonthDayNano { months: -2147483648, days: 0, nanoseconds: 0 }"
);
}
```
##########
arrow-array/src/types.rs:
##########
@@ -1140,7 +1140,7 @@ impl Date32Type {
delta: <IntervalYearMonthType as ArrowPrimitiveType>::Native,
) -> Option<<Date32Type as ArrowPrimitiveType>::Native> {
let prior = Date32Type::to_naive_date_opt(date)?;
- let months = IntervalYearMonthType::to_months(-delta);
+ let months = IntervalYearMonthType::to_months(delta.checked_neg()?);
Review Comment:
Should we add a short comment here and at the other three `checked_neg()?`
sites (lines 1229, 1333, and 1371)? A reader could take `checked_neg()?` as
rejecting a subtraction that should succeed. As I read it, `None` is the right
answer because a shift of 2^31 months (about 179 million years) is far outside
the range of `NaiveDate`, which runs from year -262143 to 262142
([numeric.rs](https://github.com/apache/arrow-rs/blob/3ef09c668303f46da22a56373bd125a320fecf3b/arrow-arith/src/numeric.rs#L1183)).
The subtraction would overflow even if the negation didn't.
```suggestion
// -i32::MIN overflows i32, but a 2^31 month shift is outside
NaiveDate's range anyway
let months = IntervalYearMonthType::to_months(delta.checked_neg()?);
```
--
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]
