choksi2212 commented on code in PR #11611:
URL: https://github.com/apache/arrow-rs/pull/11611#discussion_r4305219977
##########
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:
Done in a19611c — added the comment at all four sites. Kept it to two lines
each: `-i32::MIN` overflows `i32`, but a 2^31-month shift is outside
`NaiveDate`'s range anyway, so `None` is the right answer regardless.
Re-verified locally on the updated tree: `cargo test -p arrow-arith --lib`
256 passed / 0 failed (incl. the updated kernel test), the moved type-level
test passes under `arrow-array`, `cargo fmt --all --check` and `cargo clippy`
are clean.
##########
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:
Done in a19611c — the kernel test now asserts the full error message for
both the `IntervalYearMonth` and `IntervalMonthDayNano` deltas, on both
`Date32` and `Date64`, so a failure from a different operand or operation can
no longer pass it.
##########
arrow-arith/src/numeric.rs:
##########
@@ -2625,6 +2625,18 @@ mod tests {
<$type>::add_year_months_opt($date_fn(MIN_VALID_DATE),
-1).is_none(),
"add_year_months: overflow neg days"
);
+
+ // Negating i32::MIN months overflows i32: subtract must return
+ // None (reported as a Date arithmetic overflow by the kernels)
+ // instead of panicking in debug builds
+ assert!(
Review Comment:
Done in a19611c — moved those to `types.rs` as
`date_subtract_i32_min_months_overflows`, next to the `*_opt` implementations
they exercise.
--
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]
