You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "cht42 (via GitHub)" <gi...@apache.org> on 2026/01/12 09:28:44 UTC
[PR] Refactor Date32 manipulation functions for overflow handling [arrow-rs]
cht42 opened a new pull request, #9144:
URL: https://github.com/apache/arrow-rs/pull/9144
# Which issue does this PR close?
This PR does the same thing as what was done in #7737 to handle overflow errors gracely instead of panicking.
Part of #4456.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are these changes tested?
yes
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
If there are any breaking changes to public APIs, please call them out.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid panic on Date32 overflow [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #9144:
URL: https://github.com/apache/arrow-rs/pull/9144
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid panic on Date32 overflow [arrow-rs]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on code in PR #9144:
URL: https://github.com/apache/arrow-rs/pull/9144#discussion_r2688935911
##########
arrow-arith/src/numeric.rs:
##########
@@ -1644,23 +1631,484 @@ mod tests {
);
}
+ #[test]
+ fn test_date32_to_naive_date_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+ // Date32Type::to_naive_date_opt has boundaries determined by NaiveDate's supported range.
+ // Calculate their days values from epoch
+ let max_valid_days = (MAX_VALID_DATE - EPOCH).num_days() as i32;
+ let min_valid_days = (MIN_VALID_DATE - EPOCH).num_days() as i32;
+
+ // Verify these match the expected boundaries in milliseconds
+ assert_eq!(
+ max_valid_days, 95026236,
+ "December 31, 262142 should be 95026236 ms from epoch"
+ );
+ assert_eq!(
+ min_valid_days, -96465292,
+ "January 1, -262143 should be -96465292 ms from epoch"
+ );
+
+ // Test that the boundary dates work
+ assert!(
+ Date32Type::to_naive_date_opt(max_valid_days).is_some(),
+ "December 31, 262142 should return Some"
+ );
+ assert!(
+ Date32Type::to_naive_date_opt(min_valid_days).is_some(),
+ "January 1, -262143 should return Some"
+ );
+
+ // Test that one day beyond the boundaries fails
+ assert!(
+ Date32Type::to_naive_date_opt(max_valid_days + 1).is_none(),
+ "January 1, 262143 should return None"
+ );
+ assert!(
+ Date32Type::to_naive_date_opt(min_valid_days - 1).is_none(),
+ "December 31, -262144 should return None"
+ );
+
+ // Test some values well within the valid range
+ assert!(
+ Date32Type::to_naive_date_opt(0).is_some(),
+ "Epoch (1970-01-01) should return Some"
+ );
+ assert!(
+ Date32Type::to_naive_date_opt(YEAR_2000_DAYS).is_some(),
+ "Year 2000 should return Some"
+ );
+
+ // Test extreme values that definitely fail due to Duration constraints
+ assert!(
+ Date32Type::to_naive_date_opt(i32::MAX).is_none(),
+ "i32::MAX should return None"
+ );
+ assert!(
+ Date32Type::to_naive_date_opt(i32::MIN).is_none(),
+ "i32::MIN should return None"
+ );
+ }
+
+ #[test]
+ fn test_date32_add_year_months_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+
+ // Test normal case within valid range
+ assert!(
+ Date32Type::add_year_months_opt(YEAR_2000_DAYS, 120).is_some(),
+ "Adding 10 years to year 2000 should succeed"
+ );
+
+ // Test with moderate years that are within chrono's safe range
+ let large_year = NaiveDate::from_ymd_opt(5000, 1, 1).unwrap();
+ let large_year_days = (large_year - EPOCH).num_days() as i32;
+ assert!(
+ Date32Type::add_year_months_opt(large_year_days, 12).is_some(),
+ "Adding 12 months to year 5000 should succeed"
+ );
+
+ let neg_year = NaiveDate::from_ymd_opt(-5000, 12, 31).unwrap();
+ let neg_year_millis = (neg_year - EPOCH).num_days() as i32;
+ assert!(
+ Date32Type::add_year_months_opt(neg_year_millis, -12).is_some(),
+ "Subtracting 12 months from year -5000 should succeed"
+ );
+
+ // Test with extreme input values that would cause overflow
+ assert!(
+ Date32Type::add_year_months_opt(YEAR_2000_DAYS, i32::MAX).is_none(),
+ "Adding i32::MAX months should fail"
+ );
+ assert!(
+ Date32Type::add_year_months_opt(YEAR_2000_DAYS, i32::MIN).is_none(),
+ "Subtracting i32::MIN months should fail"
+ );
+ assert!(
+ Date32Type::add_year_months_opt(i32::MAX - 1, 1).is_none(),
+ "Adding months to i32::MAX should fail"
+ );
+ assert!(
+ Date32Type::add_year_months_opt(i32::MIN + 1, -1).is_none(),
+ "Subtracting months from i32::MIN should fail"
+ );
+
+ // Test edge case: adding zero should always work for valid dates
+ assert!(
+ Date32Type::add_year_months_opt(YEAR_2000_DAYS, 0).is_some(),
+ "Adding zero months should always succeed for valid dates"
+ );
+ }
+
+ #[test]
+ fn test_date32_add_day_time_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+ use arrow_buffer::IntervalDayTime;
+
+ let epoch = NaiveDate::from_ymd_opt(1970, 1, 1).unwrap();
+
+ // Test with a date far from the boundary but still testing the function
+ let near_max_date = NaiveDate::from_ymd_opt(200000, 12, 1).unwrap();
+ let near_max_days = (near_max_date - epoch).num_days() as i32;
+
+ // Adding 30 days should succeed
+ let interval_30_days = IntervalDayTime::new(30, 0);
+ assert!(
+ Date32Type::add_day_time_opt(near_max_days, interval_30_days).is_some(),
+ "Adding 30 days to large year should succeed"
+ );
+
+ // Adding a very large number of days should fail
+ let interval_large_days = IntervalDayTime::new(100000000, 0);
+ assert!(
+ Date32Type::add_day_time_opt(near_max_days, interval_large_days).is_none(),
+ "Adding 100M days to large year should fail"
+ );
+
+ // Test with a date far from the boundary in the negative direction
+ let near_min_date = NaiveDate::from_ymd_opt(-200000, 2, 1).unwrap();
+ let near_min_days = (near_min_date - epoch).num_days() as i32;
+
+ // Subtracting 30 days should succeed
+ let interval_minus_30_days = IntervalDayTime::new(-30, 0);
+ assert!(
+ Date32Type::add_day_time_opt(near_min_days, interval_minus_30_days).is_some(),
+ "Subtracting 30 days from large negative year should succeed"
+ );
+
+ // Subtracting a very large number of days should fail
+ let interval_minus_large_days = IntervalDayTime::new(-100000000, 0);
+ assert!(
+ Date32Type::add_day_time_opt(near_min_days, interval_minus_large_days).is_none(),
+ "Subtracting 100M days from large negative year should fail"
+ );
+
+ // Test normal case within valid range
+ let interval_1000_days = IntervalDayTime::new(1000, 12345);
+ assert!(
+ Date32Type::add_day_time_opt(YEAR_2000_DAYS, interval_1000_days).is_some(),
+ "Adding 1000 days and time to year 2000 should succeed"
+ );
+
+ // Test with extreme input values that would cause overflow
+ let interval_one_day = IntervalDayTime::new(1, 0);
+ assert!(
+ Date32Type::add_day_time_opt(i32::MAX, interval_one_day).is_none(),
+ "Adding interval to i32::MAX should fail"
+ );
+ assert!(
+ Date32Type::add_day_time_opt(i32::MIN, IntervalDayTime::new(-1, 0)).is_none(),
+ "Subtracting interval from i32::MIN should fail"
+ );
+
+ // Test with extreme interval values
+ let max_interval = IntervalDayTime::new(i32::MAX, i32::MAX);
+ assert!(
+ Date32Type::add_day_time_opt(0, max_interval).is_none(),
+ "Adding extreme interval should fail"
+ );
+
+ let min_interval = IntervalDayTime::new(i32::MIN, i32::MIN);
+ assert!(
+ Date32Type::add_day_time_opt(0, min_interval).is_none(),
+ "Adding extreme negative interval should fail"
+ );
+
+ // Test millisecond overflow within a day
+ let large_ms_interval = IntervalDayTime::new(0, i32::MAX);
+ assert!(
+ Date32Type::add_day_time_opt(YEAR_2000_DAYS, large_ms_interval).is_some(),
+ "Adding large milliseconds within valid range should succeed"
+ );
+ }
+
+ #[test]
+ fn test_date32_add_month_day_nano_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+ use arrow_buffer::IntervalMonthDayNano;
+
+ // Test with a large year that is still within chrono's safe range
+ let near_max_date = NaiveDate::from_ymd_opt(5000, 11, 1).unwrap();
+ let near_max_days = (near_max_date - EPOCH).num_days() as i32;
+
+ // Adding 1 month and 30 days should succeed
+ let interval_safe = IntervalMonthDayNano::new(1, 30, 0);
+ assert!(
+ Date32Type::add_month_day_nano_opt(near_max_days, interval_safe).is_some(),
+ "Adding 1 month 30 days to large year should succeed"
+ );
+
+ // Test edge case: adding zero should always work for valid dates
+ let zero_interval = IntervalMonthDayNano::new(0, 0, 0);
+ assert!(
+ Date32Type::add_month_day_nano_opt(YEAR_2000_DAYS, zero_interval).is_some(),
+ "Adding zero interval should always succeed for valid dates"
+ );
+
+ // Test with a negative year that is still within chrono's safe range
+ let near_min_date = NaiveDate::from_ymd_opt(-5000, 2, 28).unwrap();
+ let near_min_days = (near_min_date - EPOCH).num_days() as i32;
+
+ // Subtracting 1 month and 30 days should succeed
+ let interval_safe_neg = IntervalMonthDayNano::new(-1, -30, 0);
+ assert!(
+ Date32Type::add_month_day_nano_opt(near_min_days, interval_safe_neg).is_some(),
+ "Subtracting 1 month 30 days from large negative year should succeed"
+ );
+
+ let interval_normal = IntervalMonthDayNano::new(2, 10, 123_456_789_000);
+ assert!(
+ Date32Type::add_month_day_nano_opt(YEAR_2000_DAYS, interval_normal).is_some(),
+ "Adding 2 months, 10 days, and nanos to year 2000 should succeed"
+ );
+
+ // Test with extreme input values that would cause overflow
+ assert!(
+ Date32Type::add_month_day_nano_opt(i32::MAX, IntervalMonthDayNano::new(1, 0, 0))
+ .is_none(),
+ "Adding interval to i32::MAX should fail"
+ );
+ assert!(
+ Date32Type::add_month_day_nano_opt(i32::MIN, IntervalMonthDayNano::new(-1, 0, 0))
+ .is_none(),
+ "Subtracting interval from i32::MIN should fail"
+ );
+
+ // Test with invalid timestamp input (the _opt function should handle these gracefully)
+
+ // Test nanosecond precision (should not affect boundary since it's < 1ms)
+ let nano_interval = IntervalMonthDayNano::new(0, 0, 999_999_999);
+ assert!(
+ Date32Type::add_month_day_nano_opt(YEAR_2000_DAYS, nano_interval).is_some(),
+ "Adding nanoseconds within valid range should succeed"
+ );
+
+ // Test large nanosecond values that convert to milliseconds
+ let large_nano_interval = IntervalMonthDayNano::new(0, 0, 86_400_000_000_000); // 1 day in nanos
+ assert!(
+ Date32Type::add_month_day_nano_opt(YEAR_2000_DAYS, large_nano_interval).is_some(),
+ "Adding 1 day worth of nanoseconds should succeed"
+ );
+ }
+
+ #[test]
+ fn test_date32_subtract_year_months_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+
+ // Test with a negative year that is still within chrono's safe range
+ let near_min_date = NaiveDate::from_ymd_opt(-5000, 12, 31).unwrap();
+ let near_min_days = (near_min_date - EPOCH).num_days() as i32;
+
+ // Subtracting 12 months should succeed
+ assert!(
+ Date32Type::subtract_year_months_opt(near_min_days, 12).is_some(),
+ "Subtracting 12 months from year -5000 should succeed"
+ );
+
+ // Test edge case: subtracting zero should always work for valid dates
+ assert!(
+ Date32Type::subtract_year_months_opt(YEAR_2000_DAYS, 0).is_some(),
+ "Subtracting zero months should always succeed for valid dates"
+ );
+
+ // Test with a large year that is still within chrono's safe range
+ let near_max_date = NaiveDate::from_ymd_opt(5000, 1, 1).unwrap();
+ let near_max_days = (near_max_date - EPOCH).num_days() as i32;
+
+ // Adding 12 months (subtracting negative) should succeed
+ assert!(
+ Date32Type::subtract_year_months_opt(near_max_days, -12).is_some(),
+ "Adding 12 months to year 5000 should succeed"
+ );
+
+ assert!(
+ Date32Type::subtract_year_months_opt(YEAR_2000_DAYS, 12).is_some(),
+ "Subtracting 1 year from year 2000 should succeed"
+ );
+
+ // Test with extreme input values that would cause overflow
+ assert!(
+ Date32Type::subtract_year_months_opt(i32::MAX, -1).is_none(),
+ "Adding months to i32::MAX should fail"
+ );
+ assert!(
+ Date32Type::subtract_year_months_opt(i32::MIN, 1).is_none(),
+ "Subtracting months from i32::MIN should fail"
+ );
+
+ // Test edge case: subtracting zero should always work for valid dates
+ assert!(
+ Date32Type::subtract_year_months_opt(YEAR_2000_DAYS, 0).is_some(),
+ "Subtracting zero months should always succeed for valid dates"
+ );
+ }
+
+ #[test]
+ fn test_date32_subtract_day_time_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+ use arrow_buffer::IntervalDayTime;
+
+ // Test with a date far from the boundary in the negative direction
+ let near_min_date = NaiveDate::from_ymd_opt(-200000, 2, 1).unwrap();
+ let near_min_days = (near_min_date - EPOCH).num_days() as i32;
+
+ // Subtracting 30 days should succeed
+ let interval_30_days = IntervalDayTime::new(30, 0);
+ assert!(
+ Date32Type::subtract_day_time_opt(near_min_days, interval_30_days).is_some(),
+ "Subtracting 30 days from large negative year should succeed"
+ );
+
+ // Subtracting a very large number of days should fail
+ let interval_large_days = IntervalDayTime::new(100000000, 0);
+ assert!(
+ Date32Type::subtract_day_time_opt(near_min_days, interval_large_days).is_none(),
+ "Subtracting 100M days from large negative year should fail"
+ );
+
+ // Test with a date far from the boundary but still testing the function
+ let near_max_date = NaiveDate::from_ymd_opt(200000, 12, 1).unwrap();
+ let near_max_days = (near_max_date - EPOCH).num_days() as i32;
+
+ // Adding 30 days (subtracting negative) should succeed
+ let interval_minus_30_days = IntervalDayTime::new(-30, 0);
+ assert!(
+ Date32Type::subtract_day_time_opt(near_max_days, interval_minus_30_days).is_some(),
+ "Adding 30 days to large year should succeed"
+ );
+
+ // Adding a very large number of days should fail
+ let interval_minus_large_days = IntervalDayTime::new(-100000000, 0);
+ assert!(
+ Date32Type::subtract_day_time_opt(near_max_days, interval_minus_large_days).is_none(),
+ "Adding 100M days to large year should fail"
+ );
+
+ // Test normal case within valid range
+ let interval_1000_days = IntervalDayTime::new(1000, 12345);
+ assert!(
+ Date32Type::subtract_day_time_opt(YEAR_2000_DAYS, interval_1000_days).is_some(),
+ "Subtracting 1000 days and time from year 2000 should succeed"
+ );
+
+ // Test with extreme input values that would cause overflow
+ let interval_one_day = IntervalDayTime::new(1, 0);
+ assert!(
+ Date32Type::subtract_day_time_opt(i32::MIN, interval_one_day).is_none(),
+ "Subtracting interval from i32::MIN should fail"
+ );
+ assert!(
+ Date32Type::subtract_day_time_opt(i32::MAX, IntervalDayTime::new(-1, 0)).is_none(),
+ "Adding interval to i32::MAX should fail"
+ );
+
+ // Test with extreme interval values
+ let max_interval = IntervalDayTime::new(i32::MAX, i32::MAX);
+ assert!(
+ Date32Type::subtract_day_time_opt(0, max_interval).is_none(),
+ "Subtracting extreme interval should fail"
+ );
+
+ let min_interval = IntervalDayTime::new(i32::MIN, i32::MIN);
+ assert!(
+ Date32Type::subtract_day_time_opt(0, min_interval).is_none(),
+ "Subtracting extreme negative interval should fail"
+ );
+
+ // Test millisecond precision
+ let large_ms_interval = IntervalDayTime::new(0, i32::MAX);
+ assert!(
+ Date32Type::subtract_day_time_opt(YEAR_2000_DAYS, large_ms_interval).is_some(),
+ "Subtracting large milliseconds within valid range should succeed"
+ );
+
+ // Test edge case: subtracting zero should always work for valid dates
+ let zero_interval = IntervalDayTime::new(0, 0);
+ assert!(
+ Date32Type::subtract_day_time_opt(YEAR_2000_DAYS, zero_interval).is_some(),
+ "Subtracting zero interval should always succeed for valid dates"
+ );
+ }
+
+ #[test]
+ fn test_date32_subtract_month_day_nano_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+ use arrow_buffer::IntervalMonthDayNano;
+
+ // Test with a negative year that is still within chrono's safe range
+ let near_min_date = NaiveDate::from_ymd_opt(-5000, 2, 28).unwrap();
+ let near_min_days = (near_min_date - EPOCH).num_days() as i32;
+
+ // Subtracting 1 month and 30 days should succeed
+ let interval_safe = IntervalMonthDayNano::new(1, 30, 0);
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(near_min_days, interval_safe).is_some(),
+ "Subtracting 1 month 30 days from large negative year should succeed"
+ );
+
+ // Test edge case: subtracting zero should always work for valid dates
+ let zero_interval = IntervalMonthDayNano::new(0, 0, 0);
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(YEAR_2000_DAYS, zero_interval).is_some(),
+ "Subtracting zero interval should always succeed for valid dates"
+ );
+
+ // Test with a large year that is still within chrono's safe range
+ let near_max_date = NaiveDate::from_ymd_opt(5000, 11, 1).unwrap();
+ let near_max_days = (near_max_date - EPOCH).num_days() as i32;
+
+ // Adding 1 month and 30 days (subtracting negative) should succeed
+ let interval_safe_neg = IntervalMonthDayNano::new(-1, -30, 0);
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(near_max_days, interval_safe_neg).is_some(),
+ "Adding 1 month 30 days to large year should succeed"
+ );
+
+ let interval_normal = IntervalMonthDayNano::new(2, 10, 123_456_789_000);
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(YEAR_2000_DAYS, interval_normal).is_some(),
+ "Subtracting 2 months, 10 days, and nanos from year 2000 should succeed"
+ );
+
+ // Test with extreme input values that would cause overflow
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(i32::MIN, IntervalMonthDayNano::new(1, 0, 0))
+ .is_none(),
+ "Subtracting interval from i32::MIN should fail"
+ );
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(i32::MAX, IntervalMonthDayNano::new(-1, 0, 0))
+ .is_none(),
+ "Adding interval to i32::MAX should fail"
+ );
+
+ // Test nanosecond precision (should not affect boundary since it's < 1ms)
+ let nano_interval = IntervalMonthDayNano::new(0, 0, 999_999_999);
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(YEAR_2000_DAYS, nano_interval).is_some(),
+ "Subtracting nanoseconds within valid range should succeed"
+ );
+
+ // Test large nanosecond values that convert to milliseconds
+ let large_nano_interval = IntervalMonthDayNano::new(0, 0, 86_400_000_000_000); // 1 day in nanos
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(YEAR_2000_DAYS, large_nano_interval).is_some(),
+ "Subtracting 1 day worth of nanoseconds should succeed"
+ );
+ }
+
#[test]
fn test_date64_to_naive_date_opt_boundary_values() {
use arrow_array::types::Date64Type;
// Date64Type::to_naive_date_opt has boundaries determined by NaiveDate's supported range.
// The valid date range is from January 1, -262143 to December 31, 262142 (Gregorian calendar).
- let epoch = NaiveDate::from_ymd_opt(1970, 1, 1).unwrap();
let ms_per_day = 24 * 60 * 60 * 1000i64;
- // Define the boundary dates using NaiveDate::from_ymd_opt
- let max_valid_date = NaiveDate::from_ymd_opt(262142, 12, 31).unwrap();
- let min_valid_date = NaiveDate::from_ymd_opt(-262143, 1, 1).unwrap();
-
// Calculate their millisecond values from epoch
- let max_valid_millis = (max_valid_date - epoch).num_milliseconds();
- let min_valid_millis = (min_valid_date - epoch).num_milliseconds();
+ let max_valid_millis = (MAX_VALID_DATE - EPOCH).num_milliseconds();
+ let min_valid_millis = (MIN_VALID_DATE - EPOCH).num_milliseconds();
Review Comment:
https://github.com/apache/arrow-rs/pull/9165
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid panic on Date32 overflow [arrow-rs]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on PR #9144:
URL: https://github.com/apache/arrow-rs/pull/9144#issuecomment-3747708999
I followed @scovich recommendation and spun up https://github.com/apache/arrow-rs/pull/9165 to refactor the current tests. With that refactor, it should be easier to integrate similar tests with Date32Type, so it'd be nice to get that merged first
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Date32 manipulation functions for overflow handling [arrow-rs]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on code in PR #9144:
URL: https://github.com/apache/arrow-rs/pull/9144#discussion_r2681467028
##########
arrow-array/src/delta.rs:
##########
@@ -87,199 +86,3 @@ pub(crate) fn sub_days_datetime<Tz: TimeZone>(dt: DateTime<Tz>, days: i32) -> Op
Ordering::Less => dt.checked_add_days(Days::new(days.unsigned_abs() as u64)),
}
}
-
-#[cfg(test)]
-mod tests {
Review Comment:
those tests were added when there was a custom function used for shift_months (https://github.com/apache/arrow-rs/pull/2031/changes#diff-39c31d552939b8478fb676b059d5920a9c71b6659600348681c9ed7c03ea8485R52). Now that we use chrono functions, those tests are not necessary anymore
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid panic on Date32 overflow [arrow-rs]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on PR #9144:
URL: https://github.com/apache/arrow-rs/pull/9144#issuecomment-3752906191
updated with the new tests, had to change them a bit again to account for the types difference
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid panic on Date32 overflow [arrow-rs]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on PR #9144:
URL: https://github.com/apache/arrow-rs/pull/9144#issuecomment-3762944376
@alamb / @scovich mind taking another look ?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid panic on Date32 overflow [arrow-rs]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on code in PR #9144:
URL: https://github.com/apache/arrow-rs/pull/9144#discussion_r2700807112
##########
arrow-array/src/delta.rs:
##########
@@ -91,195 +90,175 @@ pub(crate) fn sub_days_datetime<Tz: TimeZone>(dt: DateTime<Tz>, days: i32) -> Op
#[cfg(test)]
mod tests {
- use chrono::naive::{NaiveDate, NaiveDateTime, NaiveTime};
+ use chrono::naive::NaiveDate;
use super::*;
#[test]
- fn test_shift_months() {
+ fn test_add_monts_months() {
let base = NaiveDate::from_ymd_opt(2020, 1, 31).unwrap();
assert_eq!(
- shift_months(base, 0),
- NaiveDate::from_ymd_opt(2020, 1, 31).unwrap()
+ add_months_date(base, 0),
+ NaiveDate::from_ymd_opt(2020, 1, 31)
);
assert_eq!(
- shift_months(base, 1),
- NaiveDate::from_ymd_opt(2020, 2, 29).unwrap()
+ add_months_date(base, 1),
+ NaiveDate::from_ymd_opt(2020, 2, 29)
);
assert_eq!(
- shift_months(base, 2),
- NaiveDate::from_ymd_opt(2020, 3, 31).unwrap()
+ add_months_date(base, 2),
+ NaiveDate::from_ymd_opt(2020, 3, 31)
);
assert_eq!(
- shift_months(base, 3),
- NaiveDate::from_ymd_opt(2020, 4, 30).unwrap()
+ add_months_date(base, 3),
+ NaiveDate::from_ymd_opt(2020, 4, 30)
);
assert_eq!(
- shift_months(base, 4),
- NaiveDate::from_ymd_opt(2020, 5, 31).unwrap()
+ add_months_date(base, 4),
+ NaiveDate::from_ymd_opt(2020, 5, 31)
);
assert_eq!(
- shift_months(base, 5),
- NaiveDate::from_ymd_opt(2020, 6, 30).unwrap()
+ add_months_date(base, 5),
+ NaiveDate::from_ymd_opt(2020, 6, 30)
);
assert_eq!(
- shift_months(base, 6),
- NaiveDate::from_ymd_opt(2020, 7, 31).unwrap()
+ add_months_date(base, 6),
+ NaiveDate::from_ymd_opt(2020, 7, 31)
);
assert_eq!(
- shift_months(base, 7),
- NaiveDate::from_ymd_opt(2020, 8, 31).unwrap()
+ add_months_date(base, 7),
+ NaiveDate::from_ymd_opt(2020, 8, 31)
);
assert_eq!(
- shift_months(base, 8),
- NaiveDate::from_ymd_opt(2020, 9, 30).unwrap()
+ add_months_date(base, 8),
+ NaiveDate::from_ymd_opt(2020, 9, 30)
);
assert_eq!(
- shift_months(base, 9),
- NaiveDate::from_ymd_opt(2020, 10, 31).unwrap()
+ add_months_date(base, 9),
+ NaiveDate::from_ymd_opt(2020, 10, 31)
);
assert_eq!(
- shift_months(base, 10),
- NaiveDate::from_ymd_opt(2020, 11, 30).unwrap()
+ add_months_date(base, 10),
+ NaiveDate::from_ymd_opt(2020, 11, 30)
);
assert_eq!(
- shift_months(base, 11),
- NaiveDate::from_ymd_opt(2020, 12, 31).unwrap()
+ add_months_date(base, 11),
+ NaiveDate::from_ymd_opt(2020, 12, 31)
);
assert_eq!(
- shift_months(base, 12),
- NaiveDate::from_ymd_opt(2021, 1, 31).unwrap()
+ add_months_date(base, 12),
+ NaiveDate::from_ymd_opt(2021, 1, 31)
);
assert_eq!(
- shift_months(base, 13),
- NaiveDate::from_ymd_opt(2021, 2, 28).unwrap()
+ add_months_date(base, 13),
+ NaiveDate::from_ymd_opt(2021, 2, 28)
);
assert_eq!(
- shift_months(base, -1),
- NaiveDate::from_ymd_opt(2019, 12, 31).unwrap()
+ add_months_date(base, -1),
+ NaiveDate::from_ymd_opt(2019, 12, 31)
);
assert_eq!(
- shift_months(base, -2),
- NaiveDate::from_ymd_opt(2019, 11, 30).unwrap()
+ add_months_date(base, -2),
+ NaiveDate::from_ymd_opt(2019, 11, 30)
);
assert_eq!(
- shift_months(base, -3),
- NaiveDate::from_ymd_opt(2019, 10, 31).unwrap()
+ add_months_date(base, -3),
+ NaiveDate::from_ymd_opt(2019, 10, 31)
);
assert_eq!(
- shift_months(base, -4),
- NaiveDate::from_ymd_opt(2019, 9, 30).unwrap()
+ add_months_date(base, -4),
+ NaiveDate::from_ymd_opt(2019, 9, 30)
);
assert_eq!(
- shift_months(base, -5),
- NaiveDate::from_ymd_opt(2019, 8, 31).unwrap()
+ add_months_date(base, -5),
+ NaiveDate::from_ymd_opt(2019, 8, 31)
);
assert_eq!(
- shift_months(base, -6),
- NaiveDate::from_ymd_opt(2019, 7, 31).unwrap()
+ add_months_date(base, -6),
+ NaiveDate::from_ymd_opt(2019, 7, 31)
);
assert_eq!(
- shift_months(base, -7),
- NaiveDate::from_ymd_opt(2019, 6, 30).unwrap()
+ add_months_date(base, -7),
+ NaiveDate::from_ymd_opt(2019, 6, 30)
);
assert_eq!(
- shift_months(base, -8),
- NaiveDate::from_ymd_opt(2019, 5, 31).unwrap()
+ add_months_date(base, -8),
+ NaiveDate::from_ymd_opt(2019, 5, 31)
);
assert_eq!(
- shift_months(base, -9),
- NaiveDate::from_ymd_opt(2019, 4, 30).unwrap()
+ add_months_date(base, -9),
+ NaiveDate::from_ymd_opt(2019, 4, 30)
);
assert_eq!(
- shift_months(base, -10),
- NaiveDate::from_ymd_opt(2019, 3, 31).unwrap()
+ add_months_date(base, -10),
+ NaiveDate::from_ymd_opt(2019, 3, 31)
);
assert_eq!(
- shift_months(base, -11),
- NaiveDate::from_ymd_opt(2019, 2, 28).unwrap()
+ add_months_date(base, -11),
+ NaiveDate::from_ymd_opt(2019, 2, 28)
);
assert_eq!(
- shift_months(base, -12),
- NaiveDate::from_ymd_opt(2019, 1, 31).unwrap()
+ add_months_date(base, -12),
+ NaiveDate::from_ymd_opt(2019, 1, 31)
);
assert_eq!(
- shift_months(base, -13),
- NaiveDate::from_ymd_opt(2018, 12, 31).unwrap()
+ add_months_date(base, -13),
+ NaiveDate::from_ymd_opt(2018, 12, 31)
);
assert_eq!(
- shift_months(base, 1265),
- NaiveDate::from_ymd_opt(2125, 6, 30).unwrap()
+ add_months_date(base, 1265),
+ NaiveDate::from_ymd_opt(2125, 6, 30)
);
+
+ // overflow handling
+ assert_eq!(add_months_date(base, i32::MAX), None);
+ assert_eq!(add_months_date(base, i32::MIN), None);
}
#[test]
- fn test_shift_months_with_overflow() {
+ fn test_add_months_date_with_overflow() {
let base = NaiveDate::from_ymd_opt(2020, 12, 31).unwrap();
- assert_eq!(shift_months(base, 0), base);
+ assert_eq!(add_months_date(base, 0), Some(base));
assert_eq!(
- shift_months(base, 1),
- NaiveDate::from_ymd_opt(2021, 1, 31).unwrap()
+ add_months_date(base, 1),
+ NaiveDate::from_ymd_opt(2021, 1, 31)
);
assert_eq!(
- shift_months(base, 2),
- NaiveDate::from_ymd_opt(2021, 2, 28).unwrap()
+ add_months_date(base, 2),
+ NaiveDate::from_ymd_opt(2021, 2, 28)
);
assert_eq!(
- shift_months(base, 12),
- NaiveDate::from_ymd_opt(2021, 12, 31).unwrap()
+ add_months_date(base, 12),
+ NaiveDate::from_ymd_opt(2021, 12, 31)
);
assert_eq!(
- shift_months(base, 18),
- NaiveDate::from_ymd_opt(2022, 6, 30).unwrap()
+ add_months_date(base, 18),
+ NaiveDate::from_ymd_opt(2022, 6, 30)
);
assert_eq!(
- shift_months(base, -1),
- NaiveDate::from_ymd_opt(2020, 11, 30).unwrap()
- );
- assert_eq!(
- shift_months(base, -2),
- NaiveDate::from_ymd_opt(2020, 10, 31).unwrap()
- );
- assert_eq!(
- shift_months(base, -10),
- NaiveDate::from_ymd_opt(2020, 2, 29).unwrap()
+ add_months_date(base, -1),
+ NaiveDate::from_ymd_opt(2020, 11, 30)
);
assert_eq!(
- shift_months(base, -12),
- NaiveDate::from_ymd_opt(2019, 12, 31).unwrap()
+ add_months_date(base, -2),
+ NaiveDate::from_ymd_opt(2020, 10, 31)
);
assert_eq!(
- shift_months(base, -18),
- NaiveDate::from_ymd_opt(2019, 6, 30).unwrap()
- );
- }
-
- #[test]
- fn test_shift_months_datetime() {
Review Comment:
we don't need those datetime tests anymore, `add_months_date` only accepts `NaiveDate`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid panic on Date32 overflow [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9144:
URL: https://github.com/apache/arrow-rs/pull/9144#discussion_r2687381212
##########
arrow-array/src/types.rs:
##########
@@ -904,9 +904,22 @@ impl Date32Type {
/// # Arguments
///
/// * `i` - The Date32Type to convert
+ #[deprecated(since = "58.0.0", note = "Use to_naive_date_opt instead.")]
Review Comment:
I wonder if we should split out the `#[deprecated]` annotations to a separate PR so this one can merge immediately while the other waits for next major release?
##########
arrow-arith/src/numeric.rs:
##########
@@ -1644,23 +1631,484 @@ mod tests {
);
}
+ #[test]
+ fn test_date32_to_naive_date_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+ // Date32Type::to_naive_date_opt has boundaries determined by NaiveDate's supported range.
+ // Calculate their days values from epoch
+ let max_valid_days = (MAX_VALID_DATE - EPOCH).num_days() as i32;
+ let min_valid_days = (MIN_VALID_DATE - EPOCH).num_days() as i32;
+
+ // Verify these match the expected boundaries in milliseconds
+ assert_eq!(
+ max_valid_days, 95026236,
+ "December 31, 262142 should be 95026236 ms from epoch"
+ );
+ assert_eq!(
+ min_valid_days, -96465292,
+ "January 1, -262143 should be -96465292 ms from epoch"
+ );
+
+ // Test that the boundary dates work
+ assert!(
+ Date32Type::to_naive_date_opt(max_valid_days).is_some(),
+ "December 31, 262142 should return Some"
+ );
+ assert!(
+ Date32Type::to_naive_date_opt(min_valid_days).is_some(),
+ "January 1, -262143 should return Some"
+ );
+
+ // Test that one day beyond the boundaries fails
+ assert!(
+ Date32Type::to_naive_date_opt(max_valid_days + 1).is_none(),
+ "January 1, 262143 should return None"
+ );
+ assert!(
+ Date32Type::to_naive_date_opt(min_valid_days - 1).is_none(),
+ "December 31, -262144 should return None"
+ );
+
+ // Test some values well within the valid range
+ assert!(
+ Date32Type::to_naive_date_opt(0).is_some(),
+ "Epoch (1970-01-01) should return Some"
+ );
+ assert!(
+ Date32Type::to_naive_date_opt(YEAR_2000_DAYS).is_some(),
+ "Year 2000 should return Some"
+ );
+
+ // Test extreme values that definitely fail due to Duration constraints
+ assert!(
+ Date32Type::to_naive_date_opt(i32::MAX).is_none(),
+ "i32::MAX should return None"
+ );
+ assert!(
+ Date32Type::to_naive_date_opt(i32::MIN).is_none(),
+ "i32::MIN should return None"
+ );
+ }
+
+ #[test]
+ fn test_date32_add_year_months_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+
+ // Test normal case within valid range
+ assert!(
+ Date32Type::add_year_months_opt(YEAR_2000_DAYS, 120).is_some(),
+ "Adding 10 years to year 2000 should succeed"
+ );
+
+ // Test with moderate years that are within chrono's safe range
+ let large_year = NaiveDate::from_ymd_opt(5000, 1, 1).unwrap();
+ let large_year_days = (large_year - EPOCH).num_days() as i32;
+ assert!(
+ Date32Type::add_year_months_opt(large_year_days, 12).is_some(),
+ "Adding 12 months to year 5000 should succeed"
+ );
+
+ let neg_year = NaiveDate::from_ymd_opt(-5000, 12, 31).unwrap();
+ let neg_year_millis = (neg_year - EPOCH).num_days() as i32;
+ assert!(
+ Date32Type::add_year_months_opt(neg_year_millis, -12).is_some(),
+ "Subtracting 12 months from year -5000 should succeed"
+ );
+
+ // Test with extreme input values that would cause overflow
+ assert!(
+ Date32Type::add_year_months_opt(YEAR_2000_DAYS, i32::MAX).is_none(),
+ "Adding i32::MAX months should fail"
+ );
+ assert!(
+ Date32Type::add_year_months_opt(YEAR_2000_DAYS, i32::MIN).is_none(),
+ "Subtracting i32::MIN months should fail"
+ );
+ assert!(
+ Date32Type::add_year_months_opt(i32::MAX - 1, 1).is_none(),
+ "Adding months to i32::MAX should fail"
+ );
+ assert!(
+ Date32Type::add_year_months_opt(i32::MIN + 1, -1).is_none(),
+ "Subtracting months from i32::MIN should fail"
+ );
+
+ // Test edge case: adding zero should always work for valid dates
+ assert!(
+ Date32Type::add_year_months_opt(YEAR_2000_DAYS, 0).is_some(),
+ "Adding zero months should always succeed for valid dates"
+ );
+ }
+
+ #[test]
+ fn test_date32_add_day_time_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+ use arrow_buffer::IntervalDayTime;
+
+ let epoch = NaiveDate::from_ymd_opt(1970, 1, 1).unwrap();
+
+ // Test with a date far from the boundary but still testing the function
+ let near_max_date = NaiveDate::from_ymd_opt(200000, 12, 1).unwrap();
+ let near_max_days = (near_max_date - epoch).num_days() as i32;
+
+ // Adding 30 days should succeed
+ let interval_30_days = IntervalDayTime::new(30, 0);
+ assert!(
+ Date32Type::add_day_time_opt(near_max_days, interval_30_days).is_some(),
+ "Adding 30 days to large year should succeed"
+ );
+
+ // Adding a very large number of days should fail
+ let interval_large_days = IntervalDayTime::new(100000000, 0);
+ assert!(
+ Date32Type::add_day_time_opt(near_max_days, interval_large_days).is_none(),
+ "Adding 100M days to large year should fail"
+ );
+
+ // Test with a date far from the boundary in the negative direction
+ let near_min_date = NaiveDate::from_ymd_opt(-200000, 2, 1).unwrap();
+ let near_min_days = (near_min_date - epoch).num_days() as i32;
+
+ // Subtracting 30 days should succeed
+ let interval_minus_30_days = IntervalDayTime::new(-30, 0);
+ assert!(
+ Date32Type::add_day_time_opt(near_min_days, interval_minus_30_days).is_some(),
+ "Subtracting 30 days from large negative year should succeed"
+ );
+
+ // Subtracting a very large number of days should fail
+ let interval_minus_large_days = IntervalDayTime::new(-100000000, 0);
+ assert!(
+ Date32Type::add_day_time_opt(near_min_days, interval_minus_large_days).is_none(),
+ "Subtracting 100M days from large negative year should fail"
+ );
+
+ // Test normal case within valid range
+ let interval_1000_days = IntervalDayTime::new(1000, 12345);
+ assert!(
+ Date32Type::add_day_time_opt(YEAR_2000_DAYS, interval_1000_days).is_some(),
+ "Adding 1000 days and time to year 2000 should succeed"
+ );
+
+ // Test with extreme input values that would cause overflow
+ let interval_one_day = IntervalDayTime::new(1, 0);
+ assert!(
+ Date32Type::add_day_time_opt(i32::MAX, interval_one_day).is_none(),
+ "Adding interval to i32::MAX should fail"
+ );
+ assert!(
+ Date32Type::add_day_time_opt(i32::MIN, IntervalDayTime::new(-1, 0)).is_none(),
+ "Subtracting interval from i32::MIN should fail"
+ );
+
+ // Test with extreme interval values
+ let max_interval = IntervalDayTime::new(i32::MAX, i32::MAX);
+ assert!(
+ Date32Type::add_day_time_opt(0, max_interval).is_none(),
+ "Adding extreme interval should fail"
+ );
+
+ let min_interval = IntervalDayTime::new(i32::MIN, i32::MIN);
+ assert!(
+ Date32Type::add_day_time_opt(0, min_interval).is_none(),
+ "Adding extreme negative interval should fail"
+ );
+
+ // Test millisecond overflow within a day
+ let large_ms_interval = IntervalDayTime::new(0, i32::MAX);
+ assert!(
+ Date32Type::add_day_time_opt(YEAR_2000_DAYS, large_ms_interval).is_some(),
+ "Adding large milliseconds within valid range should succeed"
+ );
+ }
+
+ #[test]
+ fn test_date32_add_month_day_nano_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+ use arrow_buffer::IntervalMonthDayNano;
+
+ // Test with a large year that is still within chrono's safe range
+ let near_max_date = NaiveDate::from_ymd_opt(5000, 11, 1).unwrap();
+ let near_max_days = (near_max_date - EPOCH).num_days() as i32;
+
+ // Adding 1 month and 30 days should succeed
+ let interval_safe = IntervalMonthDayNano::new(1, 30, 0);
+ assert!(
+ Date32Type::add_month_day_nano_opt(near_max_days, interval_safe).is_some(),
+ "Adding 1 month 30 days to large year should succeed"
+ );
+
+ // Test edge case: adding zero should always work for valid dates
+ let zero_interval = IntervalMonthDayNano::new(0, 0, 0);
+ assert!(
+ Date32Type::add_month_day_nano_opt(YEAR_2000_DAYS, zero_interval).is_some(),
+ "Adding zero interval should always succeed for valid dates"
+ );
+
+ // Test with a negative year that is still within chrono's safe range
+ let near_min_date = NaiveDate::from_ymd_opt(-5000, 2, 28).unwrap();
+ let near_min_days = (near_min_date - EPOCH).num_days() as i32;
+
+ // Subtracting 1 month and 30 days should succeed
+ let interval_safe_neg = IntervalMonthDayNano::new(-1, -30, 0);
+ assert!(
+ Date32Type::add_month_day_nano_opt(near_min_days, interval_safe_neg).is_some(),
+ "Subtracting 1 month 30 days from large negative year should succeed"
+ );
+
+ let interval_normal = IntervalMonthDayNano::new(2, 10, 123_456_789_000);
+ assert!(
+ Date32Type::add_month_day_nano_opt(YEAR_2000_DAYS, interval_normal).is_some(),
+ "Adding 2 months, 10 days, and nanos to year 2000 should succeed"
+ );
+
+ // Test with extreme input values that would cause overflow
+ assert!(
+ Date32Type::add_month_day_nano_opt(i32::MAX, IntervalMonthDayNano::new(1, 0, 0))
+ .is_none(),
+ "Adding interval to i32::MAX should fail"
+ );
+ assert!(
+ Date32Type::add_month_day_nano_opt(i32::MIN, IntervalMonthDayNano::new(-1, 0, 0))
+ .is_none(),
+ "Subtracting interval from i32::MIN should fail"
+ );
+
+ // Test with invalid timestamp input (the _opt function should handle these gracefully)
+
+ // Test nanosecond precision (should not affect boundary since it's < 1ms)
+ let nano_interval = IntervalMonthDayNano::new(0, 0, 999_999_999);
+ assert!(
+ Date32Type::add_month_day_nano_opt(YEAR_2000_DAYS, nano_interval).is_some(),
+ "Adding nanoseconds within valid range should succeed"
+ );
+
+ // Test large nanosecond values that convert to milliseconds
+ let large_nano_interval = IntervalMonthDayNano::new(0, 0, 86_400_000_000_000); // 1 day in nanos
+ assert!(
+ Date32Type::add_month_day_nano_opt(YEAR_2000_DAYS, large_nano_interval).is_some(),
+ "Adding 1 day worth of nanoseconds should succeed"
+ );
+ }
+
+ #[test]
+ fn test_date32_subtract_year_months_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+
+ // Test with a negative year that is still within chrono's safe range
+ let near_min_date = NaiveDate::from_ymd_opt(-5000, 12, 31).unwrap();
+ let near_min_days = (near_min_date - EPOCH).num_days() as i32;
+
+ // Subtracting 12 months should succeed
+ assert!(
+ Date32Type::subtract_year_months_opt(near_min_days, 12).is_some(),
+ "Subtracting 12 months from year -5000 should succeed"
+ );
+
+ // Test edge case: subtracting zero should always work for valid dates
+ assert!(
+ Date32Type::subtract_year_months_opt(YEAR_2000_DAYS, 0).is_some(),
+ "Subtracting zero months should always succeed for valid dates"
+ );
+
+ // Test with a large year that is still within chrono's safe range
+ let near_max_date = NaiveDate::from_ymd_opt(5000, 1, 1).unwrap();
+ let near_max_days = (near_max_date - EPOCH).num_days() as i32;
+
+ // Adding 12 months (subtracting negative) should succeed
+ assert!(
+ Date32Type::subtract_year_months_opt(near_max_days, -12).is_some(),
+ "Adding 12 months to year 5000 should succeed"
+ );
+
+ assert!(
+ Date32Type::subtract_year_months_opt(YEAR_2000_DAYS, 12).is_some(),
+ "Subtracting 1 year from year 2000 should succeed"
+ );
+
+ // Test with extreme input values that would cause overflow
+ assert!(
+ Date32Type::subtract_year_months_opt(i32::MAX, -1).is_none(),
+ "Adding months to i32::MAX should fail"
+ );
+ assert!(
+ Date32Type::subtract_year_months_opt(i32::MIN, 1).is_none(),
+ "Subtracting months from i32::MIN should fail"
+ );
+
+ // Test edge case: subtracting zero should always work for valid dates
+ assert!(
+ Date32Type::subtract_year_months_opt(YEAR_2000_DAYS, 0).is_some(),
+ "Subtracting zero months should always succeed for valid dates"
+ );
+ }
+
+ #[test]
+ fn test_date32_subtract_day_time_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+ use arrow_buffer::IntervalDayTime;
+
+ // Test with a date far from the boundary in the negative direction
+ let near_min_date = NaiveDate::from_ymd_opt(-200000, 2, 1).unwrap();
+ let near_min_days = (near_min_date - EPOCH).num_days() as i32;
+
+ // Subtracting 30 days should succeed
+ let interval_30_days = IntervalDayTime::new(30, 0);
+ assert!(
+ Date32Type::subtract_day_time_opt(near_min_days, interval_30_days).is_some(),
+ "Subtracting 30 days from large negative year should succeed"
+ );
+
+ // Subtracting a very large number of days should fail
+ let interval_large_days = IntervalDayTime::new(100000000, 0);
+ assert!(
+ Date32Type::subtract_day_time_opt(near_min_days, interval_large_days).is_none(),
+ "Subtracting 100M days from large negative year should fail"
+ );
+
+ // Test with a date far from the boundary but still testing the function
+ let near_max_date = NaiveDate::from_ymd_opt(200000, 12, 1).unwrap();
+ let near_max_days = (near_max_date - EPOCH).num_days() as i32;
+
+ // Adding 30 days (subtracting negative) should succeed
+ let interval_minus_30_days = IntervalDayTime::new(-30, 0);
+ assert!(
+ Date32Type::subtract_day_time_opt(near_max_days, interval_minus_30_days).is_some(),
+ "Adding 30 days to large year should succeed"
+ );
+
+ // Adding a very large number of days should fail
+ let interval_minus_large_days = IntervalDayTime::new(-100000000, 0);
+ assert!(
+ Date32Type::subtract_day_time_opt(near_max_days, interval_minus_large_days).is_none(),
+ "Adding 100M days to large year should fail"
+ );
+
+ // Test normal case within valid range
+ let interval_1000_days = IntervalDayTime::new(1000, 12345);
+ assert!(
+ Date32Type::subtract_day_time_opt(YEAR_2000_DAYS, interval_1000_days).is_some(),
+ "Subtracting 1000 days and time from year 2000 should succeed"
+ );
+
+ // Test with extreme input values that would cause overflow
+ let interval_one_day = IntervalDayTime::new(1, 0);
+ assert!(
+ Date32Type::subtract_day_time_opt(i32::MIN, interval_one_day).is_none(),
+ "Subtracting interval from i32::MIN should fail"
+ );
+ assert!(
+ Date32Type::subtract_day_time_opt(i32::MAX, IntervalDayTime::new(-1, 0)).is_none(),
+ "Adding interval to i32::MAX should fail"
+ );
+
+ // Test with extreme interval values
+ let max_interval = IntervalDayTime::new(i32::MAX, i32::MAX);
+ assert!(
+ Date32Type::subtract_day_time_opt(0, max_interval).is_none(),
+ "Subtracting extreme interval should fail"
+ );
+
+ let min_interval = IntervalDayTime::new(i32::MIN, i32::MIN);
+ assert!(
+ Date32Type::subtract_day_time_opt(0, min_interval).is_none(),
+ "Subtracting extreme negative interval should fail"
+ );
+
+ // Test millisecond precision
+ let large_ms_interval = IntervalDayTime::new(0, i32::MAX);
+ assert!(
+ Date32Type::subtract_day_time_opt(YEAR_2000_DAYS, large_ms_interval).is_some(),
+ "Subtracting large milliseconds within valid range should succeed"
+ );
+
+ // Test edge case: subtracting zero should always work for valid dates
+ let zero_interval = IntervalDayTime::new(0, 0);
+ assert!(
+ Date32Type::subtract_day_time_opt(YEAR_2000_DAYS, zero_interval).is_some(),
+ "Subtracting zero interval should always succeed for valid dates"
+ );
+ }
+
+ #[test]
+ fn test_date32_subtract_month_day_nano_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+ use arrow_buffer::IntervalMonthDayNano;
+
+ // Test with a negative year that is still within chrono's safe range
+ let near_min_date = NaiveDate::from_ymd_opt(-5000, 2, 28).unwrap();
+ let near_min_days = (near_min_date - EPOCH).num_days() as i32;
+
+ // Subtracting 1 month and 30 days should succeed
+ let interval_safe = IntervalMonthDayNano::new(1, 30, 0);
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(near_min_days, interval_safe).is_some(),
+ "Subtracting 1 month 30 days from large negative year should succeed"
+ );
+
+ // Test edge case: subtracting zero should always work for valid dates
+ let zero_interval = IntervalMonthDayNano::new(0, 0, 0);
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(YEAR_2000_DAYS, zero_interval).is_some(),
+ "Subtracting zero interval should always succeed for valid dates"
+ );
+
+ // Test with a large year that is still within chrono's safe range
+ let near_max_date = NaiveDate::from_ymd_opt(5000, 11, 1).unwrap();
+ let near_max_days = (near_max_date - EPOCH).num_days() as i32;
+
+ // Adding 1 month and 30 days (subtracting negative) should succeed
+ let interval_safe_neg = IntervalMonthDayNano::new(-1, -30, 0);
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(near_max_days, interval_safe_neg).is_some(),
+ "Adding 1 month 30 days to large year should succeed"
+ );
+
+ let interval_normal = IntervalMonthDayNano::new(2, 10, 123_456_789_000);
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(YEAR_2000_DAYS, interval_normal).is_some(),
+ "Subtracting 2 months, 10 days, and nanos from year 2000 should succeed"
+ );
+
+ // Test with extreme input values that would cause overflow
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(i32::MIN, IntervalMonthDayNano::new(1, 0, 0))
+ .is_none(),
+ "Subtracting interval from i32::MIN should fail"
+ );
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(i32::MAX, IntervalMonthDayNano::new(-1, 0, 0))
+ .is_none(),
+ "Adding interval to i32::MAX should fail"
+ );
+
+ // Test nanosecond precision (should not affect boundary since it's < 1ms)
+ let nano_interval = IntervalMonthDayNano::new(0, 0, 999_999_999);
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(YEAR_2000_DAYS, nano_interval).is_some(),
+ "Subtracting nanoseconds within valid range should succeed"
+ );
+
+ // Test large nanosecond values that convert to milliseconds
+ let large_nano_interval = IntervalMonthDayNano::new(0, 0, 86_400_000_000_000); // 1 day in nanos
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(YEAR_2000_DAYS, large_nano_interval).is_some(),
+ "Subtracting 1 day worth of nanoseconds should succeed"
+ );
+ }
+
#[test]
fn test_date64_to_naive_date_opt_boundary_values() {
use arrow_array::types::Date64Type;
// Date64Type::to_naive_date_opt has boundaries determined by NaiveDate's supported range.
// The valid date range is from January 1, -262143 to December 31, 262142 (Gregorian calendar).
- let epoch = NaiveDate::from_ymd_opt(1970, 1, 1).unwrap();
let ms_per_day = 24 * 60 * 60 * 1000i64;
- // Define the boundary dates using NaiveDate::from_ymd_opt
- let max_valid_date = NaiveDate::from_ymd_opt(262142, 12, 31).unwrap();
- let min_valid_date = NaiveDate::from_ymd_opt(-262143, 1, 1).unwrap();
-
// Calculate their millisecond values from epoch
- let max_valid_millis = (max_valid_date - epoch).num_milliseconds();
- let min_valid_millis = (min_valid_date - epoch).num_milliseconds();
+ let max_valid_millis = (MAX_VALID_DATE - EPOCH).num_milliseconds();
+ let min_valid_millis = (MIN_VALID_DATE - EPOCH).num_milliseconds();
Review Comment:
Changes like this seem unrelated, better to split out as a "prefactor" PR that can merge quickly? Then this PR would be easier to review by not mixing refactor with bug fixes and API changes.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid panic on Date32 overflow [arrow-rs]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on code in PR #9144:
URL: https://github.com/apache/arrow-rs/pull/9144#discussion_r2692996008
##########
arrow-array/src/delta.rs:
##########
@@ -87,199 +86,3 @@ pub(crate) fn sub_days_datetime<Tz: TimeZone>(dt: DateTime<Tz>, days: i32) -> Op
Ordering::Less => dt.checked_add_days(Days::new(days.unsigned_abs() as u64)),
}
}
-
-#[cfg(test)]
-mod tests {
Review Comment:
sounds good, added them back. Removed the datetime ones as we don't need them and added a couple test for the integer overflow handling
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid panic on Date32 overflow [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9144:
URL: https://github.com/apache/arrow-rs/pull/9144#discussion_r2692219591
##########
arrow-array/src/types.rs:
##########
@@ -904,9 +904,22 @@ impl Date32Type {
/// # Arguments
///
/// * `i` - The Date32Type to convert
+ #[deprecated(since = "58.0.0", note = "Use to_naive_date_opt instead.")]
Review Comment:
(FWIW the next release will be major so it is less of a concern)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid panic on Date32 overflow [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9144:
URL: https://github.com/apache/arrow-rs/pull/9144#issuecomment-3751986970
> I followed @scovich recommendation and spun up #9165 to refactor the current tests. With that refactor, it should be easier to integrate similar tests with Date32Type, so it'd be nice to get that merged first
Merged! Now there are some conflicts with this PR to address however
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid panic on Date32 overflow [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9144:
URL: https://github.com/apache/arrow-rs/pull/9144#discussion_r2687421295
##########
arrow-array/src/types.rs:
##########
@@ -941,15 +975,36 @@ impl Date32Type {
///
/// * `date` - The date on which to perform the operation
/// * `delta` - The interval to add
+ #[deprecated(
+ since = "58.0.0",
+ note = "Use `add_day_time_opt` instead, which returns an Option to handle overflow."
+ )]
pub fn add_day_time(
date: <Date32Type as ArrowPrimitiveType>::Native,
delta: <IntervalDayTimeType as ArrowPrimitiveType>::Native,
) -> <Date32Type as ArrowPrimitiveType>::Native {
+ Self::add_day_time_opt(date, delta).unwrap_or_else(|| {
+ panic!("Date32Type::add_day_time overflowed for date: {date}, delta: {delta:?}",)
+ })
+ }
+
+ /// Adds the given IntervalDayTimeType to an arrow Date32Type
+ ///
+ /// # Arguments
+ ///
+ /// * `date` - The date on which to perform the operation
+ /// * `delta` - The interval to add
+ ///
+ /// Returns `Some(Date32Type)` if it fits, `None` otherwise.
+ pub fn add_day_time_opt(
+ date: <Date32Type as ArrowPrimitiveType>::Native,
+ delta: <IntervalDayTimeType as ArrowPrimitiveType>::Native,
+ ) -> Option<<Date32Type as ArrowPrimitiveType>::Native> {
let (days, ms) = IntervalDayTimeType::to_parts(delta);
- let res = Date32Type::to_naive_date(date);
- let res = res.add(Duration::try_days(days as i64).unwrap());
- let res = res.add(Duration::try_milliseconds(ms as i64).unwrap());
- Date32Type::from_naive_date(res)
+ let res = Date32Type::to_naive_date_opt(date)?;
+ let res = res.checked_add_signed(Duration::try_days(days as i64)?)?;
Review Comment:
I checked that `add` just calls into `checked_add_signed` and panics. Thus this is equivalent
https://docs.rs/chrono/latest/src/chrono/naive/date/mod.rs.html#1952
impl Add<TimeDelta> for NaiveDate {
1953 type Output = NaiveDate;
1954
1955 #[inline]
1956 fn add(self, rhs: TimeDelta) -> NaiveDate {
1957 self.checked_add_signed(rhs).expect("`NaiveDate + TimeDelta` overflowed")
1958 }
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1029,7 +1029,7 @@ fn typed_value_to_variant<'a>(
generic_conversion_single_value!(
Date32Type,
as_primitive,
- Date32Type::to_naive_date,
+ |v| Date32Type::to_naive_date_opt(v).unwrap(),
Review Comment:
This used to panic implicitly and now the panic'ing is explicit. I think that is an improvement
##########
arrow-array/src/delta.rs:
##########
@@ -87,199 +86,3 @@ pub(crate) fn sub_days_datetime<Tz: TimeZone>(dt: DateTime<Tz>, days: i32) -> Op
Ordering::Less => dt.checked_add_days(Days::new(days.unsigned_abs() as u64)),
}
}
-
-#[cfg(test)]
-mod tests {
Review Comment:
However, this PR also adds `add_months_date` that is quite similar to `shift_months` -- do you think it is valuable to port the test to use `add_months_date` instead?
That would make it easier to ensure we are not changing behavior (esp with respect to over flow handling / truncation)
##########
arrow-arith/src/numeric.rs:
##########
@@ -1644,23 +1631,484 @@ mod tests {
);
}
+ #[test]
+ fn test_date32_to_naive_date_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+ // Date32Type::to_naive_date_opt has boundaries determined by NaiveDate's supported range.
+ // Calculate their days values from epoch
+ let max_valid_days = (MAX_VALID_DATE - EPOCH).num_days() as i32;
+ let min_valid_days = (MIN_VALID_DATE - EPOCH).num_days() as i32;
+
+ // Verify these match the expected boundaries in milliseconds
+ assert_eq!(
+ max_valid_days, 95026236,
+ "December 31, 262142 should be 95026236 ms from epoch"
+ );
+ assert_eq!(
+ min_valid_days, -96465292,
+ "January 1, -262143 should be -96465292 ms from epoch"
+ );
+
+ // Test that the boundary dates work
+ assert!(
+ Date32Type::to_naive_date_opt(max_valid_days).is_some(),
+ "December 31, 262142 should return Some"
+ );
+ assert!(
+ Date32Type::to_naive_date_opt(min_valid_days).is_some(),
+ "January 1, -262143 should return Some"
+ );
+
+ // Test that one day beyond the boundaries fails
+ assert!(
+ Date32Type::to_naive_date_opt(max_valid_days + 1).is_none(),
+ "January 1, 262143 should return None"
+ );
+ assert!(
+ Date32Type::to_naive_date_opt(min_valid_days - 1).is_none(),
+ "December 31, -262144 should return None"
+ );
+
+ // Test some values well within the valid range
+ assert!(
+ Date32Type::to_naive_date_opt(0).is_some(),
+ "Epoch (1970-01-01) should return Some"
+ );
+ assert!(
+ Date32Type::to_naive_date_opt(YEAR_2000_DAYS).is_some(),
+ "Year 2000 should return Some"
+ );
+
+ // Test extreme values that definitely fail due to Duration constraints
+ assert!(
+ Date32Type::to_naive_date_opt(i32::MAX).is_none(),
+ "i32::MAX should return None"
+ );
+ assert!(
+ Date32Type::to_naive_date_opt(i32::MIN).is_none(),
+ "i32::MIN should return None"
+ );
+ }
+
+ #[test]
+ fn test_date32_add_year_months_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+
+ // Test normal case within valid range
+ assert!(
+ Date32Type::add_year_months_opt(YEAR_2000_DAYS, 120).is_some(),
+ "Adding 10 years to year 2000 should succeed"
+ );
+
+ // Test with moderate years that are within chrono's safe range
+ let large_year = NaiveDate::from_ymd_opt(5000, 1, 1).unwrap();
+ let large_year_days = (large_year - EPOCH).num_days() as i32;
+ assert!(
+ Date32Type::add_year_months_opt(large_year_days, 12).is_some(),
+ "Adding 12 months to year 5000 should succeed"
+ );
+
+ let neg_year = NaiveDate::from_ymd_opt(-5000, 12, 31).unwrap();
+ let neg_year_millis = (neg_year - EPOCH).num_days() as i32;
+ assert!(
+ Date32Type::add_year_months_opt(neg_year_millis, -12).is_some(),
+ "Subtracting 12 months from year -5000 should succeed"
+ );
+
+ // Test with extreme input values that would cause overflow
+ assert!(
+ Date32Type::add_year_months_opt(YEAR_2000_DAYS, i32::MAX).is_none(),
+ "Adding i32::MAX months should fail"
+ );
+ assert!(
+ Date32Type::add_year_months_opt(YEAR_2000_DAYS, i32::MIN).is_none(),
+ "Subtracting i32::MIN months should fail"
+ );
+ assert!(
+ Date32Type::add_year_months_opt(i32::MAX - 1, 1).is_none(),
+ "Adding months to i32::MAX should fail"
+ );
+ assert!(
+ Date32Type::add_year_months_opt(i32::MIN + 1, -1).is_none(),
+ "Subtracting months from i32::MIN should fail"
+ );
+
+ // Test edge case: adding zero should always work for valid dates
+ assert!(
+ Date32Type::add_year_months_opt(YEAR_2000_DAYS, 0).is_some(),
+ "Adding zero months should always succeed for valid dates"
+ );
+ }
+
+ #[test]
+ fn test_date32_add_day_time_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+ use arrow_buffer::IntervalDayTime;
+
+ let epoch = NaiveDate::from_ymd_opt(1970, 1, 1).unwrap();
+
+ // Test with a date far from the boundary but still testing the function
+ let near_max_date = NaiveDate::from_ymd_opt(200000, 12, 1).unwrap();
+ let near_max_days = (near_max_date - epoch).num_days() as i32;
+
+ // Adding 30 days should succeed
+ let interval_30_days = IntervalDayTime::new(30, 0);
+ assert!(
+ Date32Type::add_day_time_opt(near_max_days, interval_30_days).is_some(),
+ "Adding 30 days to large year should succeed"
+ );
+
+ // Adding a very large number of days should fail
+ let interval_large_days = IntervalDayTime::new(100000000, 0);
+ assert!(
+ Date32Type::add_day_time_opt(near_max_days, interval_large_days).is_none(),
+ "Adding 100M days to large year should fail"
+ );
+
+ // Test with a date far from the boundary in the negative direction
+ let near_min_date = NaiveDate::from_ymd_opt(-200000, 2, 1).unwrap();
+ let near_min_days = (near_min_date - epoch).num_days() as i32;
+
+ // Subtracting 30 days should succeed
+ let interval_minus_30_days = IntervalDayTime::new(-30, 0);
+ assert!(
+ Date32Type::add_day_time_opt(near_min_days, interval_minus_30_days).is_some(),
+ "Subtracting 30 days from large negative year should succeed"
+ );
+
+ // Subtracting a very large number of days should fail
+ let interval_minus_large_days = IntervalDayTime::new(-100000000, 0);
+ assert!(
+ Date32Type::add_day_time_opt(near_min_days, interval_minus_large_days).is_none(),
+ "Subtracting 100M days from large negative year should fail"
+ );
+
+ // Test normal case within valid range
+ let interval_1000_days = IntervalDayTime::new(1000, 12345);
+ assert!(
+ Date32Type::add_day_time_opt(YEAR_2000_DAYS, interval_1000_days).is_some(),
+ "Adding 1000 days and time to year 2000 should succeed"
+ );
+
+ // Test with extreme input values that would cause overflow
+ let interval_one_day = IntervalDayTime::new(1, 0);
+ assert!(
+ Date32Type::add_day_time_opt(i32::MAX, interval_one_day).is_none(),
+ "Adding interval to i32::MAX should fail"
+ );
+ assert!(
+ Date32Type::add_day_time_opt(i32::MIN, IntervalDayTime::new(-1, 0)).is_none(),
+ "Subtracting interval from i32::MIN should fail"
+ );
+
+ // Test with extreme interval values
+ let max_interval = IntervalDayTime::new(i32::MAX, i32::MAX);
+ assert!(
+ Date32Type::add_day_time_opt(0, max_interval).is_none(),
+ "Adding extreme interval should fail"
+ );
+
+ let min_interval = IntervalDayTime::new(i32::MIN, i32::MIN);
+ assert!(
+ Date32Type::add_day_time_opt(0, min_interval).is_none(),
+ "Adding extreme negative interval should fail"
+ );
+
+ // Test millisecond overflow within a day
+ let large_ms_interval = IntervalDayTime::new(0, i32::MAX);
+ assert!(
+ Date32Type::add_day_time_opt(YEAR_2000_DAYS, large_ms_interval).is_some(),
+ "Adding large milliseconds within valid range should succeed"
+ );
+ }
+
+ #[test]
+ fn test_date32_add_month_day_nano_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+ use arrow_buffer::IntervalMonthDayNano;
+
+ // Test with a large year that is still within chrono's safe range
+ let near_max_date = NaiveDate::from_ymd_opt(5000, 11, 1).unwrap();
+ let near_max_days = (near_max_date - EPOCH).num_days() as i32;
+
+ // Adding 1 month and 30 days should succeed
+ let interval_safe = IntervalMonthDayNano::new(1, 30, 0);
+ assert!(
+ Date32Type::add_month_day_nano_opt(near_max_days, interval_safe).is_some(),
+ "Adding 1 month 30 days to large year should succeed"
+ );
+
+ // Test edge case: adding zero should always work for valid dates
+ let zero_interval = IntervalMonthDayNano::new(0, 0, 0);
+ assert!(
+ Date32Type::add_month_day_nano_opt(YEAR_2000_DAYS, zero_interval).is_some(),
+ "Adding zero interval should always succeed for valid dates"
+ );
+
+ // Test with a negative year that is still within chrono's safe range
+ let near_min_date = NaiveDate::from_ymd_opt(-5000, 2, 28).unwrap();
+ let near_min_days = (near_min_date - EPOCH).num_days() as i32;
+
+ // Subtracting 1 month and 30 days should succeed
+ let interval_safe_neg = IntervalMonthDayNano::new(-1, -30, 0);
+ assert!(
+ Date32Type::add_month_day_nano_opt(near_min_days, interval_safe_neg).is_some(),
+ "Subtracting 1 month 30 days from large negative year should succeed"
+ );
+
+ let interval_normal = IntervalMonthDayNano::new(2, 10, 123_456_789_000);
+ assert!(
+ Date32Type::add_month_day_nano_opt(YEAR_2000_DAYS, interval_normal).is_some(),
+ "Adding 2 months, 10 days, and nanos to year 2000 should succeed"
+ );
+
+ // Test with extreme input values that would cause overflow
+ assert!(
+ Date32Type::add_month_day_nano_opt(i32::MAX, IntervalMonthDayNano::new(1, 0, 0))
+ .is_none(),
+ "Adding interval to i32::MAX should fail"
+ );
+ assert!(
+ Date32Type::add_month_day_nano_opt(i32::MIN, IntervalMonthDayNano::new(-1, 0, 0))
+ .is_none(),
+ "Subtracting interval from i32::MIN should fail"
+ );
+
+ // Test with invalid timestamp input (the _opt function should handle these gracefully)
+
+ // Test nanosecond precision (should not affect boundary since it's < 1ms)
+ let nano_interval = IntervalMonthDayNano::new(0, 0, 999_999_999);
+ assert!(
+ Date32Type::add_month_day_nano_opt(YEAR_2000_DAYS, nano_interval).is_some(),
+ "Adding nanoseconds within valid range should succeed"
+ );
+
+ // Test large nanosecond values that convert to milliseconds
+ let large_nano_interval = IntervalMonthDayNano::new(0, 0, 86_400_000_000_000); // 1 day in nanos
+ assert!(
+ Date32Type::add_month_day_nano_opt(YEAR_2000_DAYS, large_nano_interval).is_some(),
+ "Adding 1 day worth of nanoseconds should succeed"
+ );
+ }
+
+ #[test]
+ fn test_date32_subtract_year_months_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+
+ // Test with a negative year that is still within chrono's safe range
+ let near_min_date = NaiveDate::from_ymd_opt(-5000, 12, 31).unwrap();
+ let near_min_days = (near_min_date - EPOCH).num_days() as i32;
+
+ // Subtracting 12 months should succeed
+ assert!(
+ Date32Type::subtract_year_months_opt(near_min_days, 12).is_some(),
+ "Subtracting 12 months from year -5000 should succeed"
+ );
+
+ // Test edge case: subtracting zero should always work for valid dates
+ assert!(
+ Date32Type::subtract_year_months_opt(YEAR_2000_DAYS, 0).is_some(),
+ "Subtracting zero months should always succeed for valid dates"
+ );
+
+ // Test with a large year that is still within chrono's safe range
+ let near_max_date = NaiveDate::from_ymd_opt(5000, 1, 1).unwrap();
+ let near_max_days = (near_max_date - EPOCH).num_days() as i32;
+
+ // Adding 12 months (subtracting negative) should succeed
+ assert!(
+ Date32Type::subtract_year_months_opt(near_max_days, -12).is_some(),
+ "Adding 12 months to year 5000 should succeed"
+ );
+
+ assert!(
+ Date32Type::subtract_year_months_opt(YEAR_2000_DAYS, 12).is_some(),
+ "Subtracting 1 year from year 2000 should succeed"
+ );
+
+ // Test with extreme input values that would cause overflow
+ assert!(
+ Date32Type::subtract_year_months_opt(i32::MAX, -1).is_none(),
+ "Adding months to i32::MAX should fail"
+ );
+ assert!(
+ Date32Type::subtract_year_months_opt(i32::MIN, 1).is_none(),
+ "Subtracting months from i32::MIN should fail"
+ );
+
+ // Test edge case: subtracting zero should always work for valid dates
+ assert!(
+ Date32Type::subtract_year_months_opt(YEAR_2000_DAYS, 0).is_some(),
+ "Subtracting zero months should always succeed for valid dates"
+ );
+ }
+
+ #[test]
+ fn test_date32_subtract_day_time_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+ use arrow_buffer::IntervalDayTime;
+
+ // Test with a date far from the boundary in the negative direction
+ let near_min_date = NaiveDate::from_ymd_opt(-200000, 2, 1).unwrap();
+ let near_min_days = (near_min_date - EPOCH).num_days() as i32;
+
+ // Subtracting 30 days should succeed
+ let interval_30_days = IntervalDayTime::new(30, 0);
+ assert!(
+ Date32Type::subtract_day_time_opt(near_min_days, interval_30_days).is_some(),
+ "Subtracting 30 days from large negative year should succeed"
+ );
+
+ // Subtracting a very large number of days should fail
+ let interval_large_days = IntervalDayTime::new(100000000, 0);
+ assert!(
+ Date32Type::subtract_day_time_opt(near_min_days, interval_large_days).is_none(),
+ "Subtracting 100M days from large negative year should fail"
+ );
+
+ // Test with a date far from the boundary but still testing the function
+ let near_max_date = NaiveDate::from_ymd_opt(200000, 12, 1).unwrap();
+ let near_max_days = (near_max_date - EPOCH).num_days() as i32;
+
+ // Adding 30 days (subtracting negative) should succeed
+ let interval_minus_30_days = IntervalDayTime::new(-30, 0);
+ assert!(
+ Date32Type::subtract_day_time_opt(near_max_days, interval_minus_30_days).is_some(),
+ "Adding 30 days to large year should succeed"
+ );
+
+ // Adding a very large number of days should fail
+ let interval_minus_large_days = IntervalDayTime::new(-100000000, 0);
+ assert!(
+ Date32Type::subtract_day_time_opt(near_max_days, interval_minus_large_days).is_none(),
+ "Adding 100M days to large year should fail"
+ );
+
+ // Test normal case within valid range
+ let interval_1000_days = IntervalDayTime::new(1000, 12345);
+ assert!(
+ Date32Type::subtract_day_time_opt(YEAR_2000_DAYS, interval_1000_days).is_some(),
+ "Subtracting 1000 days and time from year 2000 should succeed"
+ );
+
+ // Test with extreme input values that would cause overflow
+ let interval_one_day = IntervalDayTime::new(1, 0);
+ assert!(
+ Date32Type::subtract_day_time_opt(i32::MIN, interval_one_day).is_none(),
+ "Subtracting interval from i32::MIN should fail"
+ );
+ assert!(
+ Date32Type::subtract_day_time_opt(i32::MAX, IntervalDayTime::new(-1, 0)).is_none(),
+ "Adding interval to i32::MAX should fail"
+ );
+
+ // Test with extreme interval values
+ let max_interval = IntervalDayTime::new(i32::MAX, i32::MAX);
+ assert!(
+ Date32Type::subtract_day_time_opt(0, max_interval).is_none(),
+ "Subtracting extreme interval should fail"
+ );
+
+ let min_interval = IntervalDayTime::new(i32::MIN, i32::MIN);
+ assert!(
+ Date32Type::subtract_day_time_opt(0, min_interval).is_none(),
+ "Subtracting extreme negative interval should fail"
+ );
+
+ // Test millisecond precision
+ let large_ms_interval = IntervalDayTime::new(0, i32::MAX);
+ assert!(
+ Date32Type::subtract_day_time_opt(YEAR_2000_DAYS, large_ms_interval).is_some(),
+ "Subtracting large milliseconds within valid range should succeed"
+ );
+
+ // Test edge case: subtracting zero should always work for valid dates
+ let zero_interval = IntervalDayTime::new(0, 0);
+ assert!(
+ Date32Type::subtract_day_time_opt(YEAR_2000_DAYS, zero_interval).is_some(),
+ "Subtracting zero interval should always succeed for valid dates"
+ );
+ }
+
+ #[test]
+ fn test_date32_subtract_month_day_nano_opt_boundary_values() {
+ use arrow_array::types::Date32Type;
+ use arrow_buffer::IntervalMonthDayNano;
+
+ // Test with a negative year that is still within chrono's safe range
+ let near_min_date = NaiveDate::from_ymd_opt(-5000, 2, 28).unwrap();
+ let near_min_days = (near_min_date - EPOCH).num_days() as i32;
+
+ // Subtracting 1 month and 30 days should succeed
+ let interval_safe = IntervalMonthDayNano::new(1, 30, 0);
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(near_min_days, interval_safe).is_some(),
+ "Subtracting 1 month 30 days from large negative year should succeed"
+ );
+
+ // Test edge case: subtracting zero should always work for valid dates
+ let zero_interval = IntervalMonthDayNano::new(0, 0, 0);
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(YEAR_2000_DAYS, zero_interval).is_some(),
+ "Subtracting zero interval should always succeed for valid dates"
+ );
+
+ // Test with a large year that is still within chrono's safe range
+ let near_max_date = NaiveDate::from_ymd_opt(5000, 11, 1).unwrap();
+ let near_max_days = (near_max_date - EPOCH).num_days() as i32;
+
+ // Adding 1 month and 30 days (subtracting negative) should succeed
+ let interval_safe_neg = IntervalMonthDayNano::new(-1, -30, 0);
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(near_max_days, interval_safe_neg).is_some(),
+ "Adding 1 month 30 days to large year should succeed"
+ );
+
+ let interval_normal = IntervalMonthDayNano::new(2, 10, 123_456_789_000);
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(YEAR_2000_DAYS, interval_normal).is_some(),
+ "Subtracting 2 months, 10 days, and nanos from year 2000 should succeed"
+ );
+
+ // Test with extreme input values that would cause overflow
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(i32::MIN, IntervalMonthDayNano::new(1, 0, 0))
+ .is_none(),
+ "Subtracting interval from i32::MIN should fail"
+ );
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(i32::MAX, IntervalMonthDayNano::new(-1, 0, 0))
+ .is_none(),
+ "Adding interval to i32::MAX should fail"
+ );
+
+ // Test nanosecond precision (should not affect boundary since it's < 1ms)
+ let nano_interval = IntervalMonthDayNano::new(0, 0, 999_999_999);
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(YEAR_2000_DAYS, nano_interval).is_some(),
+ "Subtracting nanoseconds within valid range should succeed"
+ );
+
+ // Test large nanosecond values that convert to milliseconds
+ let large_nano_interval = IntervalMonthDayNano::new(0, 0, 86_400_000_000_000); // 1 day in nanos
+ assert!(
+ Date32Type::subtract_month_day_nano_opt(YEAR_2000_DAYS, large_nano_interval).is_some(),
+ "Subtracting 1 day worth of nanoseconds should succeed"
+ );
+ }
+
#[test]
fn test_date64_to_naive_date_opt_boundary_values() {
use arrow_array::types::Date64Type;
// Date64Type::to_naive_date_opt has boundaries determined by NaiveDate's supported range.
// The valid date range is from January 1, -262143 to December 31, 262142 (Gregorian calendar).
- let epoch = NaiveDate::from_ymd_opt(1970, 1, 1).unwrap();
Review Comment:
thank you for cleaning up this repetition
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid panic on Date32 overflow [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9144:
URL: https://github.com/apache/arrow-rs/pull/9144#issuecomment-3745648543
Thank you for your contribution @cht42 🙏
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org