comphead commented on code in PR #7530:
URL: https://github.com/apache/datafusion-comet/pull/7530#discussion_r4565705841
##########
native/spark-expr/src/iceberg_funcs/temporal.rs:
##########
@@ -116,35 +117,13 @@ fn micros_to_hours(micros: i64) -> i32 {
/// `DateTimeUtil.daysToYears`: whole calendar years between the epoch and the
day, floored.
fn days_to_years(days: i32) -> i32 {
- civil_from_days(days).0 - UNIX_EPOCH_YEAR
+ (civil_from_days(days.into()).0 - UNIX_EPOCH_YEAR) as i32
}
/// `DateTimeUtil.daysToMonths`: whole calendar months between the epoch and
the day, floored.
fn days_to_months(days: i32) -> i32 {
- let (year, month0) = civil_from_days(days);
- (year - UNIX_EPOCH_YEAR) * 12 + month0
-}
-
-/// Splits an epoch day into its proleptic Gregorian `(year, month0)`,
following Howard Hinnant's
-/// `civil_from_days`. The intermediates are `i64` so that every `i32` epoch
day is in range, which
-/// is what `LocalDate` gives Iceberg; the widest results, at `i32::MIN` and
`i32::MAX` days, are
-/// years -5877641 and 5881580, so both the year and the month count still fit
in an `i32`.
-fn civil_from_days(days: i32) -> (i32, i32) {
- // Shift the epoch to 0000-03-01 so that the leap day falls at the end of
the year.
- let shifted = days as i64 + 719_468;
- let era = shifted.div_euclid(146_097);
- let day_of_era = shifted.rem_euclid(146_097); // [0, 146096]
- let year_of_era =
- (day_of_era - day_of_era / 1460 + day_of_era / 36_524 - day_of_era /
146_096) / 365; // [0, 399]
- let day_of_year = day_of_era - (365 * year_of_era + year_of_era / 4 -
year_of_era / 100); // [0, 365]
- let shifted_month = (5 * day_of_year + 2) / 153; // [0, 11], March is 0
- let month = if shifted_month < 10 {
- shifted_month + 3
- } else {
- shifted_month - 9
- };
- let year = era * 400 + year_of_era + i64::from(month <= 2);
- (year as i32, (month - 1) as i32)
+ let (year, month, _) = civil_from_days(days.into());
+ ((year - UNIX_EPOCH_YEAR) * 12 + month - 1) as i32
}
Review Comment:
The removed `civil_from_days` doc was the only note on why these `as i32`
casts cannot truncate: at `i32::MIN` and `i32::MAX` days the years are -5877641
and 5881580, so both the year and the month count fit in an `i32`. Now that the
shared function returns `i64`, could that sentence move here?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
