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