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/14 04:49:14 UTC
[PR] refactor: streamline date64 tests [arrow-rs]
cht42 opened a new pull request, #9165:
URL: https://github.com/apache/arrow-rs/pull/9165
# Which issue does this PR close?
- Closes #N/A (internal refactoring - no issue)
# Rationale for this change
Noticed while writing tests in #9144, that the current tests could be re-written to be easier to read/re-use.
⚠️ FIY, I used claude to refactor those tests, I read the changes and we are keeping the same test cases.
The Date64 boundary tests in `arrow-arith/src/numeric.rs` had significant code duplication. Each test function for Date64 operations (`to_naive_date_opt`, `add_year_months_opt`, `subtract_year_months_opt`, `add_day_time_opt`, `subtract_day_time_opt`, `add_month_day_nano_opt`, `subtract_month_day_nano_opt`) repeated similar setup code and boundary checks, making the test suite harder to maintain and extend.
# What changes are included in this PR?
This PR refactors the Date64 boundary tests by:
1. **Introducing shared constants** for commonly used values:
- `MAX_VALID_DATE`, `MIN_VALID_DATE`, `EPOCH` - NaiveDate constants for chrono's valid date range
2. **Adding utility functions** to reduce repetition:
- `date_to_millis(year, month, day)` - converts a date to milliseconds from epoch
- `max_valid_millis()`, `min_valid_millis()`, `year_2000_millis()` - common millisecond values
3. **Consolidating similar test patterns** into parameterized helper functions:
- `test_year_month_op()` - tests `add_year_months_opt` and `subtract_year_months_opt`
- `test_day_time_op()` - tests `add_day_time_opt` and `subtract_day_time_opt`
- `test_month_day_nano_op()` - tests `add_month_day_nano_opt` and `subtract_month_day_nano_opt`
4. **Reducing 8 separate test functions to 4** while maintaining the same test coverage
Net result: **-297 lines** (163 added, 460 removed) with equivalent functionality.
# Are these changes tested?
Yes - this is a refactoring of existing tests. The same boundary conditions and edge cases are still tested, just organized more efficiently. Running `cargo test` confirms all tests pass.
# Are there any user-facing changes?
No. This is an internal test refactoring with no changes to public APIs or functionality.
--
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: streamline date64 tests [arrow-rs]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on PR #9165:
URL: https://github.com/apache/arrow-rs/pull/9165#issuecomment-3750594966
@scovich this part here is testing the close to limit -> going over limit cases https://github.com/apache/arrow-rs/pull/9165/changes#diff-94fd6c08249d7fd9b2364e8b8cd66163e6d0e0c0a8d04007b7a693dfd3e919d5R1742-R1763.
--
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: streamline date64 tests [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #9165:
URL: 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] refactor: streamline date64 tests [arrow-rs]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on code in PR #9165:
URL: https://github.com/apache/arrow-rs/pull/9165#discussion_r2691298259
##########
arrow-arith/src/numeric.rs:
##########
@@ -1645,534 +1662,209 @@ mod tests {
}
#[test]
- fn test_date64_to_naive_date_opt_boundary_values() {
+ fn test_date64_to_naive_date_opt_boundaries() {
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;
+ const MS_PER_DAY: i64 = 24 * 60 * 60 * 1000;
- // 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();
+ // Verify boundary millisecond values
+ assert_eq!(MAX_VALID_MILLIS, 8210266790400000i64);
+ assert_eq!(MIN_VALID_MILLIS, -8334601228800000i64);
- // 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();
+ // Valid boundary dates work
+ assert!(Date64Type::to_naive_date_opt(MAX_VALID_MILLIS).is_some());
+ assert!(Date64Type::to_naive_date_opt(MIN_VALID_MILLIS).is_some());
- // Verify these match the expected boundaries in milliseconds
- assert_eq!(
- max_valid_millis, 8210266790400000i64,
- "December 31, 262142 should be 8210266790400000 ms from epoch"
- );
- assert_eq!(
- min_valid_millis, -8334601228800000i64,
- "January 1, -262143 should be -8334601228800000 ms from epoch"
- );
+ // Beyond boundaries fail
+ assert!(Date64Type::to_naive_date_opt(MAX_VALID_MILLIS + MS_PER_DAY).is_none());
+ assert!(Date64Type::to_naive_date_opt(MIN_VALID_MILLIS - MS_PER_DAY).is_none());
- // Test that the boundary dates work
- assert!(
- Date64Type::to_naive_date_opt(max_valid_millis).is_some(),
- "December 31, 262142 should return Some"
- );
- assert!(
- Date64Type::to_naive_date_opt(min_valid_millis).is_some(),
- "January 1, -262143 should return Some"
- );
+ // Extreme values fail
+ assert!(Date64Type::to_naive_date_opt(i64::MAX).is_none());
+ assert!(Date64Type::to_naive_date_opt(i64::MIN).is_none());
- // Test that one day beyond the boundaries fails
- assert!(
- Date64Type::to_naive_date_opt(max_valid_millis + ms_per_day).is_none(),
- "January 1, 262143 should return None"
- );
- assert!(
- Date64Type::to_naive_date_opt(min_valid_millis - ms_per_day).is_none(),
- "December 31, -262144 should return None"
- );
-
- // Test some values well within the valid range
- assert!(
- Date64Type::to_naive_date_opt(0).is_some(),
- "Epoch (1970-01-01) should return Some"
- );
- let year_2000 = NaiveDate::from_ymd_opt(2000, 1, 1).unwrap();
- let year_2000_millis = (year_2000 - epoch).num_milliseconds();
- assert!(
- Date64Type::to_naive_date_opt(year_2000_millis).is_some(),
- "Year 2000 should return Some"
- );
-
- // Test extreme values that definitely fail due to Duration constraints
- assert!(
- Date64Type::to_naive_date_opt(i64::MAX).is_none(),
- "i64::MAX should return None"
- );
- assert!(
- Date64Type::to_naive_date_opt(i64::MIN).is_none(),
- "i64::MIN should return None"
- );
+ // Common values work
+ assert!(Date64Type::to_naive_date_opt(0).is_some());
+ assert!(Date64Type::to_naive_date_opt(YEAR_2000_MILLIS).is_some());
}
- #[test]
- fn test_date64_add_year_months_opt_boundary_values() {
- use arrow_array::types::Date64Type;
-
- let epoch = NaiveDate::from_ymd_opt(1970, 1, 1).unwrap();
-
- // Test normal case within valid range
- let year_2000 = NaiveDate::from_ymd_opt(2000, 1, 1).unwrap();
- let year_2000_millis = (year_2000 - epoch).num_milliseconds();
- assert!(
- Date64Type::add_year_months_opt(year_2000_millis, 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_millis = (large_year - epoch).num_milliseconds();
- assert!(
- Date64Type::add_year_months_opt(large_year_millis, 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_milliseconds();
+ fn test_year_month_op<F>(op: F, op_name: &str)
+ where
+ F: Fn(i64, i32) -> Option<i64>,
+ {
+ // Normal operations succeed
+ assert!(op(YEAR_2000_MILLIS, 120).is_some(), "{op_name}: normal add");
assert!(
- Date64Type::add_year_months_opt(neg_year_millis, -12).is_some(),
- "Subtracting 12 months from year -5000 should succeed"
+ op(YEAR_2000_MILLIS, 0).is_some(),
+ "{op_name}: zero interval"
);
- // Test with extreme input values that would cause overflow
- assert!(
- Date64Type::add_year_months_opt(i64::MAX, 1).is_none(),
- "Adding months to i64::MAX should fail"
- );
- assert!(
- Date64Type::add_year_months_opt(i64::MIN, -1).is_none(),
- "Subtracting months from i64::MIN should fail"
- );
+ // Large but valid years work
+ let large_year = date_to_millis(5000, 1, 1);
+ let neg_year = date_to_millis(-5000, 12, 31);
+ assert!(op(large_year, 12).is_some(), "{op_name}: large year");
+ assert!(op(neg_year, -12).is_some(), "{op_name}: negative year");
- // Test edge case: adding zero should always work for valid dates
- assert!(
- Date64Type::add_year_months_opt(year_2000_millis, 0).is_some(),
- "Adding zero months should always succeed for valid dates"
- );
+ // Extreme base values fail
+ assert!(op(i64::MAX, 1).is_none(), "{op_name}: i64::MAX base");
+ assert!(op(i64::MIN, -1).is_none(), "{op_name}: i64::MIN base");
}
#[test]
- fn test_date64_add_day_time_opt_boundary_values() {
+ fn test_date64_year_month_operations() {
use arrow_array::types::Date64Type;
- 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_millis = (near_max_date - epoch).num_milliseconds();
- // Adding 30 days should succeed
- let interval_30_days = IntervalDayTime::new(30, 0);
- assert!(
- Date64Type::add_day_time_opt(near_max_millis, 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!(
- Date64Type::add_day_time_opt(near_max_millis, 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_millis = (near_min_date - epoch).num_milliseconds();
-
- // Subtracting 30 days should succeed
- let interval_minus_30_days = IntervalDayTime::new(-30, 0);
- assert!(
- Date64Type::add_day_time_opt(near_min_millis, 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!(
- Date64Type::add_day_time_opt(near_min_millis, interval_minus_large_days).is_none(),
- "Subtracting 100M days from large negative year should fail"
- );
-
- // Test normal case within valid range
- let year_2000 = NaiveDate::from_ymd_opt(2000, 1, 1).unwrap();
- let year_2000_millis = (year_2000 - epoch).num_milliseconds();
- let interval_1000_days = IntervalDayTime::new(1000, 12345);
- assert!(
- Date64Type::add_day_time_opt(year_2000_millis, 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!(
- Date64Type::add_day_time_opt(i64::MAX, interval_one_day).is_none(),
- "Adding interval to i64::MAX should fail"
- );
- assert!(
- Date64Type::add_day_time_opt(i64::MIN, IntervalDayTime::new(-1, 0)).is_none(),
- "Subtracting interval from i64::MIN should fail"
- );
-
- // Test with extreme interval values
- let max_interval = IntervalDayTime::new(i32::MAX, i32::MAX);
- assert!(
- Date64Type::add_day_time_opt(0, max_interval).is_none(),
- "Adding extreme interval should fail"
- );
-
- let min_interval = IntervalDayTime::new(i32::MIN, i32::MIN);
- assert!(
- Date64Type::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!(
- Date64Type::add_day_time_opt(year_2000_millis, large_ms_interval).is_some(),
- "Adding large milliseconds within valid range should succeed"
- );
+ test_year_month_op(Date64Type::add_year_months_opt, "add_year_months");
+ test_year_month_op(Date64Type::subtract_year_months_opt, "subtract_year_months");
}
- #[test]
- fn test_date64_add_month_day_nano_opt_boundary_values() {
- use arrow_array::types::Date64Type;
- use arrow_buffer::IntervalMonthDayNano;
-
- let epoch = NaiveDate::from_ymd_opt(1970, 1, 1).unwrap();
-
- // 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_millis = (near_max_date - epoch).num_milliseconds();
-
- // Adding 1 month and 30 days should succeed
- let interval_safe = IntervalMonthDayNano::new(1, 30, 0);
- assert!(
- Date64Type::add_month_day_nano_opt(near_max_millis, interval_safe).is_some(),
- "Adding 1 month 30 days to large year should succeed"
- );
-
- // Test normal case within valid range
- let year_2000 = NaiveDate::from_ymd_opt(2000, 1, 1).unwrap();
- let year_2000_millis = (year_2000 - epoch).num_milliseconds();
-
- // Test edge case: adding zero should always work for valid dates
- let zero_interval = IntervalMonthDayNano::new(0, 0, 0);
- assert!(
- Date64Type::add_month_day_nano_opt(year_2000_millis, 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_millis = (near_min_date - epoch).num_milliseconds();
-
- // Subtracting 1 month and 30 days should succeed
- let interval_safe_neg = IntervalMonthDayNano::new(-1, -30, 0);
- assert!(
- Date64Type::add_month_day_nano_opt(near_min_millis, interval_safe_neg).is_some(),
- "Subtracting 1 month 30 days from large negative year should succeed"
- );
+ fn test_day_time_op<F>(op: F, op_name: &str, is_subtract: bool)
+ where
+ F: Fn(i64, IntervalDayTime) -> Option<i64>,
+ {
+ let near_max = date_to_millis(200000, 12, 1);
+ let near_min = date_to_millis(-200000, 2, 1);
+
+ // Moderate intervals succeed
+ assert!(
+ op(near_max, IntervalDayTime::new(30, 0)).is_some(),
+ "{op_name}: +30 days"
+ );
+ assert!(
+ op(near_min, IntervalDayTime::new(-30, 0)).is_some(),
+ "{op_name}: -30 days"
+ );
+ assert!(
+ op(YEAR_2000_MILLIS, IntervalDayTime::new(1000, 12345)).is_some(),
+ "{op_name}: normal"
+ );
+
+ // Large intervals that would overflow - behavior differs for add vs subtract
+ // For add: near_max + huge_days overflows, near_min + huge_neg_days overflows
+ // For subtract: near_min - huge_days overflows, near_max - huge_neg_days overflows
+ if is_subtract {
Review Comment:
here @scovich (linking is behaving weirdly for me)
--
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: streamline date64 tests [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9165:
URL: https://github.com/apache/arrow-rs/pull/9165#discussion_r2692255713
##########
arrow-arith/src/numeric.rs:
##########
@@ -1645,534 +1662,203 @@ mod tests {
}
#[test]
- fn test_date64_to_naive_date_opt_boundary_values() {
+ fn test_date64_to_naive_date_opt_boundaries() {
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;
+ const MS_PER_DAY: i64 = 24 * 60 * 60 * 1000;
- // 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();
-
- // Verify these match the expected boundaries in milliseconds
- assert_eq!(
- max_valid_millis, 8210266790400000i64,
- "December 31, 262142 should be 8210266790400000 ms from epoch"
- );
- assert_eq!(
- min_valid_millis, -8334601228800000i64,
- "January 1, -262143 should be -8334601228800000 ms from epoch"
- );
+ // Verify boundary millisecond values
+ assert_eq!(MAX_VALID_MILLIS, 8210266790400000i64);
+ assert_eq!(MIN_VALID_MILLIS, -8334601228800000i64);
- // Test that the boundary dates work
- assert!(
- Date64Type::to_naive_date_opt(max_valid_millis).is_some(),
- "December 31, 262142 should return Some"
- );
- assert!(
- Date64Type::to_naive_date_opt(min_valid_millis).is_some(),
- "January 1, -262143 should return Some"
- );
+ // Valid boundary dates work
+ assert!(Date64Type::to_naive_date_opt(MAX_VALID_MILLIS).is_some());
+ assert!(Date64Type::to_naive_date_opt(MIN_VALID_MILLIS).is_some());
- // Test that one day beyond the boundaries fails
- assert!(
- Date64Type::to_naive_date_opt(max_valid_millis + ms_per_day).is_none(),
- "January 1, 262143 should return None"
- );
- assert!(
- Date64Type::to_naive_date_opt(min_valid_millis - ms_per_day).is_none(),
- "December 31, -262144 should return None"
- );
+ // Beyond boundaries fail
+ assert!(Date64Type::to_naive_date_opt(MAX_VALID_MILLIS + MS_PER_DAY).is_none());
+ assert!(Date64Type::to_naive_date_opt(MIN_VALID_MILLIS - MS_PER_DAY).is_none());
- // Test some values well within the valid range
- assert!(
- Date64Type::to_naive_date_opt(0).is_some(),
- "Epoch (1970-01-01) should return Some"
- );
- let year_2000 = NaiveDate::from_ymd_opt(2000, 1, 1).unwrap();
- let year_2000_millis = (year_2000 - epoch).num_milliseconds();
- assert!(
- Date64Type::to_naive_date_opt(year_2000_millis).is_some(),
- "Year 2000 should return Some"
- );
+ // Extreme values fail
+ assert!(Date64Type::to_naive_date_opt(i64::MAX).is_none());
+ assert!(Date64Type::to_naive_date_opt(i64::MIN).is_none());
- // Test extreme values that definitely fail due to Duration constraints
- assert!(
- Date64Type::to_naive_date_opt(i64::MAX).is_none(),
- "i64::MAX should return None"
- );
- assert!(
- Date64Type::to_naive_date_opt(i64::MIN).is_none(),
- "i64::MIN should return None"
- );
+ // Common values work
+ assert!(Date64Type::to_naive_date_opt(0).is_some());
+ assert!(Date64Type::to_naive_date_opt(YEAR_2000_MILLIS).is_some());
}
- #[test]
- fn test_date64_add_year_months_opt_boundary_values() {
- use arrow_array::types::Date64Type;
-
- let epoch = NaiveDate::from_ymd_opt(1970, 1, 1).unwrap();
-
- // Test normal case within valid range
- let year_2000 = NaiveDate::from_ymd_opt(2000, 1, 1).unwrap();
- let year_2000_millis = (year_2000 - epoch).num_milliseconds();
- assert!(
- Date64Type::add_year_months_opt(year_2000_millis, 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_millis = (large_year - epoch).num_milliseconds();
- assert!(
- Date64Type::add_year_months_opt(large_year_millis, 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_milliseconds();
- assert!(
- Date64Type::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!(
- Date64Type::add_year_months_opt(i64::MAX, 1).is_none(),
Review Comment:
I didn't see any corresponding coverage for i64::MIN and i64::MAX in the helpers but then I see the discussion between @scovich and @cht42 about this point and I think it makes sense to avoid this
--
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: streamline date64 tests [arrow-rs]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on code in PR #9165:
URL: https://github.com/apache/arrow-rs/pull/9165#discussion_r2691533513
##########
arrow-arith/src/numeric.rs:
##########
@@ -1645,534 +1662,209 @@ mod tests {
}
#[test]
- fn test_date64_to_naive_date_opt_boundary_values() {
+ fn test_date64_to_naive_date_opt_boundaries() {
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;
+ const MS_PER_DAY: i64 = 24 * 60 * 60 * 1000;
- // 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();
+ // Verify boundary millisecond values
+ assert_eq!(MAX_VALID_MILLIS, 8210266790400000i64);
+ assert_eq!(MIN_VALID_MILLIS, -8334601228800000i64);
- // 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();
+ // Valid boundary dates work
+ assert!(Date64Type::to_naive_date_opt(MAX_VALID_MILLIS).is_some());
+ assert!(Date64Type::to_naive_date_opt(MIN_VALID_MILLIS).is_some());
- // Verify these match the expected boundaries in milliseconds
- assert_eq!(
- max_valid_millis, 8210266790400000i64,
- "December 31, 262142 should be 8210266790400000 ms from epoch"
- );
- assert_eq!(
- min_valid_millis, -8334601228800000i64,
- "January 1, -262143 should be -8334601228800000 ms from epoch"
- );
+ // Beyond boundaries fail
+ assert!(Date64Type::to_naive_date_opt(MAX_VALID_MILLIS + MS_PER_DAY).is_none());
+ assert!(Date64Type::to_naive_date_opt(MIN_VALID_MILLIS - MS_PER_DAY).is_none());
- // Test that the boundary dates work
- assert!(
- Date64Type::to_naive_date_opt(max_valid_millis).is_some(),
- "December 31, 262142 should return Some"
- );
- assert!(
- Date64Type::to_naive_date_opt(min_valid_millis).is_some(),
- "January 1, -262143 should return Some"
- );
+ // Extreme values fail
+ assert!(Date64Type::to_naive_date_opt(i64::MAX).is_none());
+ assert!(Date64Type::to_naive_date_opt(i64::MIN).is_none());
- // Test that one day beyond the boundaries fails
- assert!(
- Date64Type::to_naive_date_opt(max_valid_millis + ms_per_day).is_none(),
- "January 1, 262143 should return None"
- );
- assert!(
- Date64Type::to_naive_date_opt(min_valid_millis - ms_per_day).is_none(),
- "December 31, -262144 should return None"
- );
-
- // Test some values well within the valid range
- assert!(
- Date64Type::to_naive_date_opt(0).is_some(),
- "Epoch (1970-01-01) should return Some"
- );
- let year_2000 = NaiveDate::from_ymd_opt(2000, 1, 1).unwrap();
- let year_2000_millis = (year_2000 - epoch).num_milliseconds();
- assert!(
- Date64Type::to_naive_date_opt(year_2000_millis).is_some(),
- "Year 2000 should return Some"
- );
-
- // Test extreme values that definitely fail due to Duration constraints
- assert!(
- Date64Type::to_naive_date_opt(i64::MAX).is_none(),
- "i64::MAX should return None"
- );
- assert!(
- Date64Type::to_naive_date_opt(i64::MIN).is_none(),
- "i64::MIN should return None"
- );
+ // Common values work
+ assert!(Date64Type::to_naive_date_opt(0).is_some());
+ assert!(Date64Type::to_naive_date_opt(YEAR_2000_MILLIS).is_some());
}
- #[test]
- fn test_date64_add_year_months_opt_boundary_values() {
- use arrow_array::types::Date64Type;
-
- let epoch = NaiveDate::from_ymd_opt(1970, 1, 1).unwrap();
-
- // Test normal case within valid range
- let year_2000 = NaiveDate::from_ymd_opt(2000, 1, 1).unwrap();
- let year_2000_millis = (year_2000 - epoch).num_milliseconds();
- assert!(
- Date64Type::add_year_months_opt(year_2000_millis, 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_millis = (large_year - epoch).num_milliseconds();
- assert!(
- Date64Type::add_year_months_opt(large_year_millis, 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_milliseconds();
+ fn test_year_month_op<F>(op: F, op_name: &str)
+ where
+ F: Fn(i64, i32) -> Option<i64>,
+ {
+ // Normal operations succeed
+ assert!(op(YEAR_2000_MILLIS, 120).is_some(), "{op_name}: normal add");
assert!(
- Date64Type::add_year_months_opt(neg_year_millis, -12).is_some(),
- "Subtracting 12 months from year -5000 should succeed"
+ op(YEAR_2000_MILLIS, 0).is_some(),
+ "{op_name}: zero interval"
);
- // Test with extreme input values that would cause overflow
- assert!(
- Date64Type::add_year_months_opt(i64::MAX, 1).is_none(),
- "Adding months to i64::MAX should fail"
- );
- assert!(
- Date64Type::add_year_months_opt(i64::MIN, -1).is_none(),
- "Subtracting months from i64::MIN should fail"
- );
+ // Large but valid years work
+ let large_year = date_to_millis(5000, 1, 1);
+ let neg_year = date_to_millis(-5000, 12, 31);
+ assert!(op(large_year, 12).is_some(), "{op_name}: large year");
+ assert!(op(neg_year, -12).is_some(), "{op_name}: negative year");
- // Test edge case: adding zero should always work for valid dates
- assert!(
- Date64Type::add_year_months_opt(year_2000_millis, 0).is_some(),
- "Adding zero months should always succeed for valid dates"
- );
+ // Extreme base values fail
+ assert!(op(i64::MAX, 1).is_none(), "{op_name}: i64::MAX base");
+ assert!(op(i64::MIN, -1).is_none(), "{op_name}: i64::MIN base");
}
#[test]
- fn test_date64_add_day_time_opt_boundary_values() {
+ fn test_date64_year_month_operations() {
use arrow_array::types::Date64Type;
- 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_millis = (near_max_date - epoch).num_milliseconds();
- // Adding 30 days should succeed
- let interval_30_days = IntervalDayTime::new(30, 0);
- assert!(
- Date64Type::add_day_time_opt(near_max_millis, 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!(
- Date64Type::add_day_time_opt(near_max_millis, 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_millis = (near_min_date - epoch).num_milliseconds();
-
- // Subtracting 30 days should succeed
- let interval_minus_30_days = IntervalDayTime::new(-30, 0);
- assert!(
- Date64Type::add_day_time_opt(near_min_millis, 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!(
- Date64Type::add_day_time_opt(near_min_millis, interval_minus_large_days).is_none(),
- "Subtracting 100M days from large negative year should fail"
- );
-
- // Test normal case within valid range
- let year_2000 = NaiveDate::from_ymd_opt(2000, 1, 1).unwrap();
- let year_2000_millis = (year_2000 - epoch).num_milliseconds();
- let interval_1000_days = IntervalDayTime::new(1000, 12345);
- assert!(
- Date64Type::add_day_time_opt(year_2000_millis, 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!(
- Date64Type::add_day_time_opt(i64::MAX, interval_one_day).is_none(),
- "Adding interval to i64::MAX should fail"
- );
- assert!(
- Date64Type::add_day_time_opt(i64::MIN, IntervalDayTime::new(-1, 0)).is_none(),
- "Subtracting interval from i64::MIN should fail"
- );
-
- // Test with extreme interval values
- let max_interval = IntervalDayTime::new(i32::MAX, i32::MAX);
- assert!(
- Date64Type::add_day_time_opt(0, max_interval).is_none(),
- "Adding extreme interval should fail"
- );
-
- let min_interval = IntervalDayTime::new(i32::MIN, i32::MIN);
- assert!(
- Date64Type::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!(
- Date64Type::add_day_time_opt(year_2000_millis, large_ms_interval).is_some(),
- "Adding large milliseconds within valid range should succeed"
- );
+ test_year_month_op(Date64Type::add_year_months_opt, "add_year_months");
+ test_year_month_op(Date64Type::subtract_year_months_opt, "subtract_year_months");
}
- #[test]
- fn test_date64_add_month_day_nano_opt_boundary_values() {
- use arrow_array::types::Date64Type;
- use arrow_buffer::IntervalMonthDayNano;
-
- let epoch = NaiveDate::from_ymd_opt(1970, 1, 1).unwrap();
-
- // 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_millis = (near_max_date - epoch).num_milliseconds();
-
- // Adding 1 month and 30 days should succeed
- let interval_safe = IntervalMonthDayNano::new(1, 30, 0);
- assert!(
- Date64Type::add_month_day_nano_opt(near_max_millis, interval_safe).is_some(),
- "Adding 1 month 30 days to large year should succeed"
- );
-
- // Test normal case within valid range
- let year_2000 = NaiveDate::from_ymd_opt(2000, 1, 1).unwrap();
- let year_2000_millis = (year_2000 - epoch).num_milliseconds();
-
- // Test edge case: adding zero should always work for valid dates
- let zero_interval = IntervalMonthDayNano::new(0, 0, 0);
- assert!(
- Date64Type::add_month_day_nano_opt(year_2000_millis, 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_millis = (near_min_date - epoch).num_milliseconds();
-
- // Subtracting 1 month and 30 days should succeed
- let interval_safe_neg = IntervalMonthDayNano::new(-1, -30, 0);
- assert!(
- Date64Type::add_month_day_nano_opt(near_min_millis, interval_safe_neg).is_some(),
- "Subtracting 1 month 30 days from large negative year should succeed"
- );
+ fn test_day_time_op<F>(op: F, op_name: &str, is_subtract: bool)
+ where
+ F: Fn(i64, IntervalDayTime) -> Option<i64>,
+ {
+ let near_max = date_to_millis(200000, 12, 1);
+ let near_min = date_to_millis(-200000, 2, 1);
+
+ // Moderate intervals succeed
+ assert!(
+ op(near_max, IntervalDayTime::new(30, 0)).is_some(),
+ "{op_name}: +30 days"
+ );
+ assert!(
+ op(near_min, IntervalDayTime::new(-30, 0)).is_some(),
+ "{op_name}: -30 days"
+ );
+ assert!(
+ op(YEAR_2000_MILLIS, IntervalDayTime::new(1000, 12345)).is_some(),
+ "{op_name}: normal"
+ );
+
+ // Large intervals that would overflow - behavior differs for add vs subtract
+ // For add: near_max + huge_days overflows, near_min + huge_neg_days overflows
+ // For subtract: near_min - huge_days overflows, near_max - huge_neg_days overflows
+ if is_subtract {
Review Comment:
removed, you're right it wasn't useful.
--
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: streamline date64 tests [arrow-rs]
Posted by "AndreaBozzo (via GitHub)" <gi...@apache.org>.
AndreaBozzo commented on PR #9165:
URL: https://github.com/apache/arrow-rs/pull/9165#issuecomment-3750124544
> The approach looks very nice.
>
> > ⚠️ FYI, I used claude to refactor those tests, I read the changes and we are keeping the same test cases.
>
> Great use for an LLM IMO!
>
> One issue tho, which I also failed to notice when eyeballing the changes, but which claude pointed out when I asked it to check the before/after tests for equivalence:
>
> > The original tests for `subtract_day_time_opt` and `subtract_month_day_nano_opt` specifically tested:
> >
> > * Subtracting a positive interval from i64::MIN (should fail going more negative)
> > * Subtracting a negative interval from i64::MAX (should fail going more positive)
> >
> > The refactored versions test:
> >
> > * Subtracting a positive interval from i64::MAX (mathematically valid direction but extreme input)
> > * Subtracting a negative interval from i64::MIN (mathematically valid direction but extreme input)
>
> It suggested three possible ways to address this (favoring 2/):
>
> 1. Update the test logic to be add/sub aware and test the original edge cases
>
> 2. Expand the test logic to test both the original and new edge cases
>
> 3. Accept that the negative test "succeeds" (both BEFORE and AFTER) because we can't create a date from i64::MIN/MAX in the first place -- so it doesn't matter what interval we pass because the test will never get that far.
>
>
> However, 3/ raised red flags for me: There are already tests to exercise physical (integer) overflow, and based on the comments here I suspect that the original test author _intended_ to test logical overflow, where the interval shifts a valid date in a "bad" direction that takes it out of gamut. If so, the original tests were broken and the refactored ones should actually be working with MIN/MAX_VALID_MILLIS instead of ia64::MIN/MAX. At which point it would be important to also apply 1/ from the list above.
>
> It also raises the question of whether we should use `?` for intermediate operations like the date creation, or if `unwrap` is more appropriate to be sure the test failed/succeeded for the reason we think it did?
Hi, sorry for the intrusion, i approved this PR because the refactoring itself is clean and the core test coverage is preserved.
Regarding scovich's observation about the subtle difference in edge case testing (i64::MIN/MAX with positive/negative intervals): since neither i64::MIN nor i64::MAX can be converted to a valid date in the first place, the test fails at date creation before the interval logic is even reached, the behavior is effectively the same (?)
That said, if the original intent was to test *logical* overflow (a valid date shifted out of range), then both the original and refactored tests miss that case. Testing with MIN_VALID_MILLIS/MAX_VALID_MILLIS instead would be a nice improvement i super agree, even in separate pr.
This would also be a great "good first issue" for newcomers to the repo (like myself 😄) who want to improve test coverage.
--
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: streamline date64 tests [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #9165:
URL: https://github.com/apache/arrow-rs/pull/9165#discussion_r2689575902
##########
arrow-arith/src/numeric.rs:
##########
@@ -1644,535 +1649,233 @@ mod tests {
);
}
- #[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();
-
- // Verify these match the expected boundaries in milliseconds
- assert_eq!(
- max_valid_millis, 8210266790400000i64,
- "December 31, 262142 should be 8210266790400000 ms from epoch"
- );
- assert_eq!(
- min_valid_millis, -8334601228800000i64,
- "January 1, -262143 should be -8334601228800000 ms from epoch"
- );
-
- // Test that the boundary dates work
- assert!(
- Date64Type::to_naive_date_opt(max_valid_millis).is_some(),
- "December 31, 262142 should return Some"
- );
- assert!(
- Date64Type::to_naive_date_opt(min_valid_millis).is_some(),
- "January 1, -262143 should return Some"
- );
-
- // Test that one day beyond the boundaries fails
- assert!(
- Date64Type::to_naive_date_opt(max_valid_millis + ms_per_day).is_none(),
- "January 1, 262143 should return None"
- );
- assert!(
- Date64Type::to_naive_date_opt(min_valid_millis - ms_per_day).is_none(),
- "December 31, -262144 should return None"
- );
-
- // Test some values well within the valid range
- assert!(
- Date64Type::to_naive_date_opt(0).is_some(),
- "Epoch (1970-01-01) should return Some"
- );
- let year_2000 = NaiveDate::from_ymd_opt(2000, 1, 1).unwrap();
- let year_2000_millis = (year_2000 - epoch).num_milliseconds();
- assert!(
- Date64Type::to_naive_date_opt(year_2000_millis).is_some(),
- "Year 2000 should return Some"
- );
-
- // Test extreme values that definitely fail due to Duration constraints
- assert!(
- Date64Type::to_naive_date_opt(i64::MAX).is_none(),
- "i64::MAX should return None"
- );
- assert!(
- Date64Type::to_naive_date_opt(i64::MIN).is_none(),
- "i64::MIN should return None"
- );
+ fn date_to_millis(year: i32, month: u32, day: u32) -> i64 {
Review Comment:
I feel these can be made constants as well; for example:
```rust
const MAX_VALID_MILLIS: i64 = MAX_VALID_DATE
.signed_duration_since(EPOCH)
.num_milliseconds();
const MIN_VALID_MILLIS: i64 = MIN_VALID_DATE
.signed_duration_since(EPOCH)
.num_milliseconds();
const YEAR_2000_MILLIS: i64 = date_to_millis(2000, 1, 1);
const fn date_to_millis(year: i32, month: u32, day: u32) -> i64 {
let date = NaiveDate::from_ymd_opt(year, month, day).unwrap();
date.signed_duration_since(EPOCH).num_milliseconds()
}
```
Then can simplify their usages in the functions by removing the function call to a variable and just use constant directly.
--
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: streamline date64 tests [arrow-rs]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on PR #9165:
URL: https://github.com/apache/arrow-rs/pull/9165#issuecomment-3747693482
re-iterating here, that I used claude to refactor those tests, I read the changes and we are keeping the same test cases.
--
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: streamline date64 tests [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9165:
URL: https://github.com/apache/arrow-rs/pull/9165#discussion_r2691415396
##########
arrow-arith/src/numeric.rs:
##########
@@ -1645,534 +1662,209 @@ mod tests {
}
#[test]
- fn test_date64_to_naive_date_opt_boundary_values() {
+ fn test_date64_to_naive_date_opt_boundaries() {
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;
+ const MS_PER_DAY: i64 = 24 * 60 * 60 * 1000;
- // 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();
+ // Verify boundary millisecond values
+ assert_eq!(MAX_VALID_MILLIS, 8210266790400000i64);
+ assert_eq!(MIN_VALID_MILLIS, -8334601228800000i64);
- // 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();
+ // Valid boundary dates work
+ assert!(Date64Type::to_naive_date_opt(MAX_VALID_MILLIS).is_some());
+ assert!(Date64Type::to_naive_date_opt(MIN_VALID_MILLIS).is_some());
- // Verify these match the expected boundaries in milliseconds
- assert_eq!(
- max_valid_millis, 8210266790400000i64,
- "December 31, 262142 should be 8210266790400000 ms from epoch"
- );
- assert_eq!(
- min_valid_millis, -8334601228800000i64,
- "January 1, -262143 should be -8334601228800000 ms from epoch"
- );
+ // Beyond boundaries fail
+ assert!(Date64Type::to_naive_date_opt(MAX_VALID_MILLIS + MS_PER_DAY).is_none());
+ assert!(Date64Type::to_naive_date_opt(MIN_VALID_MILLIS - MS_PER_DAY).is_none());
- // Test that the boundary dates work
- assert!(
- Date64Type::to_naive_date_opt(max_valid_millis).is_some(),
- "December 31, 262142 should return Some"
- );
- assert!(
- Date64Type::to_naive_date_opt(min_valid_millis).is_some(),
- "January 1, -262143 should return Some"
- );
+ // Extreme values fail
+ assert!(Date64Type::to_naive_date_opt(i64::MAX).is_none());
+ assert!(Date64Type::to_naive_date_opt(i64::MIN).is_none());
- // Test that one day beyond the boundaries fails
- assert!(
- Date64Type::to_naive_date_opt(max_valid_millis + ms_per_day).is_none(),
- "January 1, 262143 should return None"
- );
- assert!(
- Date64Type::to_naive_date_opt(min_valid_millis - ms_per_day).is_none(),
- "December 31, -262144 should return None"
- );
-
- // Test some values well within the valid range
- assert!(
- Date64Type::to_naive_date_opt(0).is_some(),
- "Epoch (1970-01-01) should return Some"
- );
- let year_2000 = NaiveDate::from_ymd_opt(2000, 1, 1).unwrap();
- let year_2000_millis = (year_2000 - epoch).num_milliseconds();
- assert!(
- Date64Type::to_naive_date_opt(year_2000_millis).is_some(),
- "Year 2000 should return Some"
- );
-
- // Test extreme values that definitely fail due to Duration constraints
- assert!(
- Date64Type::to_naive_date_opt(i64::MAX).is_none(),
- "i64::MAX should return None"
- );
- assert!(
- Date64Type::to_naive_date_opt(i64::MIN).is_none(),
- "i64::MIN should return None"
- );
+ // Common values work
+ assert!(Date64Type::to_naive_date_opt(0).is_some());
+ assert!(Date64Type::to_naive_date_opt(YEAR_2000_MILLIS).is_some());
}
- #[test]
- fn test_date64_add_year_months_opt_boundary_values() {
- use arrow_array::types::Date64Type;
-
- let epoch = NaiveDate::from_ymd_opt(1970, 1, 1).unwrap();
-
- // Test normal case within valid range
- let year_2000 = NaiveDate::from_ymd_opt(2000, 1, 1).unwrap();
- let year_2000_millis = (year_2000 - epoch).num_milliseconds();
- assert!(
- Date64Type::add_year_months_opt(year_2000_millis, 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_millis = (large_year - epoch).num_milliseconds();
- assert!(
- Date64Type::add_year_months_opt(large_year_millis, 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_milliseconds();
+ fn test_year_month_op<F>(op: F, op_name: &str)
+ where
+ F: Fn(i64, i32) -> Option<i64>,
+ {
+ // Normal operations succeed
+ assert!(op(YEAR_2000_MILLIS, 120).is_some(), "{op_name}: normal add");
assert!(
- Date64Type::add_year_months_opt(neg_year_millis, -12).is_some(),
- "Subtracting 12 months from year -5000 should succeed"
+ op(YEAR_2000_MILLIS, 0).is_some(),
+ "{op_name}: zero interval"
);
- // Test with extreme input values that would cause overflow
- assert!(
- Date64Type::add_year_months_opt(i64::MAX, 1).is_none(),
- "Adding months to i64::MAX should fail"
- );
- assert!(
- Date64Type::add_year_months_opt(i64::MIN, -1).is_none(),
- "Subtracting months from i64::MIN should fail"
- );
+ // Large but valid years work
+ let large_year = date_to_millis(5000, 1, 1);
+ let neg_year = date_to_millis(-5000, 12, 31);
+ assert!(op(large_year, 12).is_some(), "{op_name}: large year");
+ assert!(op(neg_year, -12).is_some(), "{op_name}: negative year");
- // Test edge case: adding zero should always work for valid dates
- assert!(
- Date64Type::add_year_months_opt(year_2000_millis, 0).is_some(),
- "Adding zero months should always succeed for valid dates"
- );
+ // Extreme base values fail
+ assert!(op(i64::MAX, 1).is_none(), "{op_name}: i64::MAX base");
+ assert!(op(i64::MIN, -1).is_none(), "{op_name}: i64::MIN base");
}
#[test]
- fn test_date64_add_day_time_opt_boundary_values() {
+ fn test_date64_year_month_operations() {
use arrow_array::types::Date64Type;
- 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_millis = (near_max_date - epoch).num_milliseconds();
- // Adding 30 days should succeed
- let interval_30_days = IntervalDayTime::new(30, 0);
- assert!(
- Date64Type::add_day_time_opt(near_max_millis, 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!(
- Date64Type::add_day_time_opt(near_max_millis, 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_millis = (near_min_date - epoch).num_milliseconds();
-
- // Subtracting 30 days should succeed
- let interval_minus_30_days = IntervalDayTime::new(-30, 0);
- assert!(
- Date64Type::add_day_time_opt(near_min_millis, 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!(
- Date64Type::add_day_time_opt(near_min_millis, interval_minus_large_days).is_none(),
- "Subtracting 100M days from large negative year should fail"
- );
-
- // Test normal case within valid range
- let year_2000 = NaiveDate::from_ymd_opt(2000, 1, 1).unwrap();
- let year_2000_millis = (year_2000 - epoch).num_milliseconds();
- let interval_1000_days = IntervalDayTime::new(1000, 12345);
- assert!(
- Date64Type::add_day_time_opt(year_2000_millis, 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!(
- Date64Type::add_day_time_opt(i64::MAX, interval_one_day).is_none(),
- "Adding interval to i64::MAX should fail"
- );
- assert!(
- Date64Type::add_day_time_opt(i64::MIN, IntervalDayTime::new(-1, 0)).is_none(),
- "Subtracting interval from i64::MIN should fail"
- );
-
- // Test with extreme interval values
- let max_interval = IntervalDayTime::new(i32::MAX, i32::MAX);
- assert!(
- Date64Type::add_day_time_opt(0, max_interval).is_none(),
- "Adding extreme interval should fail"
- );
-
- let min_interval = IntervalDayTime::new(i32::MIN, i32::MIN);
- assert!(
- Date64Type::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!(
- Date64Type::add_day_time_opt(year_2000_millis, large_ms_interval).is_some(),
- "Adding large milliseconds within valid range should succeed"
- );
+ test_year_month_op(Date64Type::add_year_months_opt, "add_year_months");
+ test_year_month_op(Date64Type::subtract_year_months_opt, "subtract_year_months");
}
- #[test]
- fn test_date64_add_month_day_nano_opt_boundary_values() {
- use arrow_array::types::Date64Type;
- use arrow_buffer::IntervalMonthDayNano;
-
- let epoch = NaiveDate::from_ymd_opt(1970, 1, 1).unwrap();
-
- // 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_millis = (near_max_date - epoch).num_milliseconds();
-
- // Adding 1 month and 30 days should succeed
- let interval_safe = IntervalMonthDayNano::new(1, 30, 0);
- assert!(
- Date64Type::add_month_day_nano_opt(near_max_millis, interval_safe).is_some(),
- "Adding 1 month 30 days to large year should succeed"
- );
-
- // Test normal case within valid range
- let year_2000 = NaiveDate::from_ymd_opt(2000, 1, 1).unwrap();
- let year_2000_millis = (year_2000 - epoch).num_milliseconds();
-
- // Test edge case: adding zero should always work for valid dates
- let zero_interval = IntervalMonthDayNano::new(0, 0, 0);
- assert!(
- Date64Type::add_month_day_nano_opt(year_2000_millis, 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_millis = (near_min_date - epoch).num_milliseconds();
-
- // Subtracting 1 month and 30 days should succeed
- let interval_safe_neg = IntervalMonthDayNano::new(-1, -30, 0);
- assert!(
- Date64Type::add_month_day_nano_opt(near_min_millis, interval_safe_neg).is_some(),
- "Subtracting 1 month 30 days from large negative year should succeed"
- );
+ fn test_day_time_op<F>(op: F, op_name: &str, is_subtract: bool)
+ where
+ F: Fn(i64, IntervalDayTime) -> Option<i64>,
+ {
+ let near_max = date_to_millis(200000, 12, 1);
+ let near_min = date_to_millis(-200000, 2, 1);
+
+ // Moderate intervals succeed
+ assert!(
+ op(near_max, IntervalDayTime::new(30, 0)).is_some(),
+ "{op_name}: +30 days"
+ );
+ assert!(
+ op(near_min, IntervalDayTime::new(-30, 0)).is_some(),
+ "{op_name}: -30 days"
+ );
+ assert!(
+ op(YEAR_2000_MILLIS, IntervalDayTime::new(1000, 12345)).is_some(),
+ "{op_name}: normal"
+ );
+
+ // Large intervals that would overflow - behavior differs for add vs subtract
+ // For add: near_max + huge_days overflows, near_min + huge_neg_days overflows
+ // For subtract: near_min - huge_days overflows, near_max - huge_neg_days overflows
+ if is_subtract {
Review Comment:
Ah nice. So we do have the coverage. Is the other test in question even _useful_ then? Or do we just leave it as-is?
--
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: streamline date64 tests [arrow-rs]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on PR #9165:
URL: https://github.com/apache/arrow-rs/pull/9165#issuecomment-3747696835
@scovich can you review ?
--
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: streamline date64 tests [arrow-rs]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on code in PR #9165:
URL: https://github.com/apache/arrow-rs/pull/9165#discussion_r2689904057
##########
arrow-arith/src/numeric.rs:
##########
@@ -1644,535 +1649,233 @@ mod tests {
);
}
- #[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();
-
- // Verify these match the expected boundaries in milliseconds
- assert_eq!(
- max_valid_millis, 8210266790400000i64,
- "December 31, 262142 should be 8210266790400000 ms from epoch"
- );
- assert_eq!(
- min_valid_millis, -8334601228800000i64,
- "January 1, -262143 should be -8334601228800000 ms from epoch"
- );
-
- // Test that the boundary dates work
- assert!(
- Date64Type::to_naive_date_opt(max_valid_millis).is_some(),
- "December 31, 262142 should return Some"
- );
- assert!(
- Date64Type::to_naive_date_opt(min_valid_millis).is_some(),
- "January 1, -262143 should return Some"
- );
-
- // Test that one day beyond the boundaries fails
- assert!(
- Date64Type::to_naive_date_opt(max_valid_millis + ms_per_day).is_none(),
- "January 1, 262143 should return None"
- );
- assert!(
- Date64Type::to_naive_date_opt(min_valid_millis - ms_per_day).is_none(),
- "December 31, -262144 should return None"
- );
-
- // Test some values well within the valid range
- assert!(
- Date64Type::to_naive_date_opt(0).is_some(),
- "Epoch (1970-01-01) should return Some"
- );
- let year_2000 = NaiveDate::from_ymd_opt(2000, 1, 1).unwrap();
- let year_2000_millis = (year_2000 - epoch).num_milliseconds();
- assert!(
- Date64Type::to_naive_date_opt(year_2000_millis).is_some(),
- "Year 2000 should return Some"
- );
-
- // Test extreme values that definitely fail due to Duration constraints
- assert!(
- Date64Type::to_naive_date_opt(i64::MAX).is_none(),
- "i64::MAX should return None"
- );
- assert!(
- Date64Type::to_naive_date_opt(i64::MIN).is_none(),
- "i64::MIN should return None"
- );
+ fn date_to_millis(year: i32, month: u32, day: u32) -> i64 {
Review Comment:
updated
--
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