ump45nose commented on code in PR #7255:
URL: https://github.com/apache/datafusion-comet/pull/7255#discussion_r4401171912
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -1443,17 +1443,38 @@ fn parse_timestamp_to_micros<T: TimeZone>(
Some(days) => days,
None => return Ok(None),
};
- // Compute the timezone offset using epoch as a surrogate probe point.
- // Extreme-year timestamps are only valid with a UTC-like fixed offset
(any DST
- // zone would overflow). Using epoch gives us the standard offset.
- let epoch_probe = NaiveDate::from_ymd_opt(1970, 1, 1)
- .unwrap()
- .and_hms_opt(0, 0, 0)
- .unwrap();
- let tz_offset_secs: i64 = match tz.from_local_datetime(&epoch_probe) {
+ // Look up the zone offset at the same local civil time moved toward
the epoch
+ // by whole 400-year Gregorian cycles until the year is inside
chrono's range.
+ // Calendars (and leap days) repeat every 400 years, so the shifted
probe is a
+ // valid NaiveDate with the same month/day/time. For
America/Los_Angeles this
+ // lands in the local-mean-time era for far-past extremes (matching
Spark's
+ // ZoneRules), instead of the standard -08:00 offset that probing
1970-01-01
+ // would return. Fixed offsets and UTC are unchanged. Far-future DST
after
+ // chrono-tz's last transition (~2099) is a separate gap (#6816).
+ let mut probe_year = year;
+ while probe_year < -262144 {
+ probe_year += 400;
+ }
+ while probe_year > 262143 {
+ probe_year -= 400;
+ }
+ let probe_date =
+ NaiveDate::from_ymd_opt(probe_year as i32, timestamp_info.month,
timestamp_info.day)
+ .expect("month/day valid across a 400-year shift");
Review Comment:
Fixed. The probe now uses `1600 + year.rem_euclid(400)` so it always lands
in 1600..=1999 inside chrono 0.4.45's `[-262143, 262142]` range (and preserves
leap days across the Gregorian cycle), instead of the while-loop that could
stop on 262143 / -262144 and panic in `from_ymd_opt.expect`. The in-range
invalid-date check is aligned to the same MIN/MAX. Added
`extreme_year_chrono_bound_neighbours_do_not_panic` covering `262543-01-01`,
`-262544-01-01`, and their neighbours.
(AI-assisted reply.)
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -1443,17 +1443,38 @@ fn parse_timestamp_to_micros<T: TimeZone>(
Some(days) => days,
None => return Ok(None),
};
- // Compute the timezone offset using epoch as a surrogate probe point.
- // Extreme-year timestamps are only valid with a UTC-like fixed offset
(any DST
- // zone would overflow). Using epoch gives us the standard offset.
- let epoch_probe = NaiveDate::from_ymd_opt(1970, 1, 1)
- .unwrap()
- .and_hms_opt(0, 0, 0)
- .unwrap();
- let tz_offset_secs: i64 = match tz.from_local_datetime(&epoch_probe) {
+ // Look up the zone offset at the same local civil time moved toward
the epoch
+ // by whole 400-year Gregorian cycles until the year is inside
chrono's range.
+ // Calendars (and leap days) repeat every 400 years, so the shifted
probe is a
+ // valid NaiveDate with the same month/day/time. For
America/Los_Angeles this
+ // lands in the local-mean-time era for far-past extremes (matching
Spark's
+ // ZoneRules), instead of the standard -08:00 offset that probing
1970-01-01
+ // would return. Fixed offsets and UTC are unchanged. Far-future DST
after
+ // chrono-tz's last transition (~2099) is a separate gap (#6816).
+ let mut probe_year = year;
+ while probe_year < -262144 {
+ probe_year += 400;
+ }
+ while probe_year > 262143 {
+ probe_year -= 400;
+ }
+ let probe_date =
+ NaiveDate::from_ymd_opt(probe_year as i32, timestamp_info.month,
timestamp_info.day)
+ .expect("month/day valid across a 400-year shift");
+ let probe_naive = probe_date.and_time(naive_time);
+ let tz_offset_secs: i64 = match tz.from_local_datetime(&probe_naive) {
LocalResult::Single(dt) => dt.offset().fix().local_minus_utc() as
i64,
+ // Fall-back overlap: Spark picks the earlier (pre-transition)
offset.
LocalResult::Ambiguous(dt, _) =>
dt.offset().fix().local_minus_utc() as i64,
- LocalResult::None => 0,
+ LocalResult::None => {
+ // Spring-forward gap on the probe: use the pre-gap offset, as
above.
+ let pre = probe_naive - chrono::Duration::hours(3);
+ match tz.from_local_datetime(&pre) {
+ LocalResult::Single(dt) =>
dt.offset().fix().local_minus_utc() as i64,
+ LocalResult::Ambiguous(dt, _) =>
dt.offset().fix().local_minus_utc() as i64,
+ LocalResult::None => 0,
+ }
+ }
};
// Compute seconds since epoch via direct calendar arithmetic.
// Use i128 for the intermediate multiply-by-1_000_000 step: the
seconds value can be
Review Comment:
Kept the arms but extracted `offset_secs_at(tz, naive) -> Option<i64>` and
routed both the extreme-year probe and the main-path gap arm through it. The 3h
subtract is now `checked_sub_signed` so an extreme `NaiveDateTime` cannot
panic; a second `None` still falls back to UTC (`0`) on the extreme path (same
as before) and to `Ok(None)` on the in-range gap path. The arms are hard to hit
for this probe for most zones, but a congruent civil time can still land in a
spring-forward gap, so the helper keeps that path covered without duplicating
the three-way match.
(AI-assisted reply.)
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -3091,6 +3112,46 @@ mod tests {
);
}
+ #[test]
+ fn extreme_year_session_tz_uses_historic_offset() {
Review Comment:
Added `extreme-year session TZ uses historic zone offset` in
`CometDateTimeUtilsSuite`, which runs `checkCastToTimestampAsLong` under
`America/Los_Angeles` and `Asia/Tokyo` for `-290308-12-21 19:59:05.224192`,
`-262544-01-01 00:00:00`, and chrono-bound neighbours — so expected values come
from Spark, and the 422s LMT gap survives the Long cast. Trimmed the Rust
session-TZ unit test to drop the UTC `i64::MAX`/`MIN` and 1800 cases that
duplicated `extreme_year_boundary_test` / the in-range path; the LA micros pin
and the new chrono-bound neighbour unit test remain.
(AI-assisted reply.)
--
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]
