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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
