ump45nose commented on code in PR #7255:
URL: https://github.com/apache/datafusion-comet/pull/7255#discussion_r4412826867
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -1443,18 +1469,22 @@ 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) {
- 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,
- };
+ // Look up the zone offset at the same local civil time on a year
inside chrono's
+ // range that is congruent modulo the 400-year Gregorian cycle (leap
days match).
+ // 1600 + rem_euclid(400) lands in 1600..=1999. For
America/Los_Angeles far-past
+ // extremes this is still in the local-mean-time era (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 probe_year = (1600 + year.rem_euclid(400)) as i32;
Review Comment:
Thanks — agreed. A single `1600 + rem_euclid(400)` window preserves the
Gregorian calendar but not timezone rules, so far-future January (e.g. `262543`
→ 1743) still hit LMT (`America/Los_Angeles` `-07:52:58`, `Asia/Tokyo`
`+09:18:59`) while Spark/Java use the modern standard offsets, and some
far-past residues (e.g. `-262500` → 1900) landed after the LMT→standard
transition.
This push splits the probe era:
- far-past (`year < CHRONO_MIN_YEAR`) → `400 + rem` (400..=799, historic/LMT)
- far-future (`year > CHRONO_MAX_YEAR`) → `2000 + rem` (2000..=2399,
modern/standard)
Rust unit tests now cover `262543-01-01` and `-262500-01-01` for LA/Tokyo
(plus the existing Long.MinValue LMT case), and `CometDateTimeUtilsSuite` adds
the same Spark-backed comparisons. I could not run the JVM/SQL suite on this
box (no Maven); CI should exercise the Scala case.
Summer DST after chrono-tz's last timespan remains the separate #6816 gap.
(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");
Review Comment:
Addressed in the previous push (`25408fd`): the while-loop shift is gone in
favor of `rem_euclid(400)` with a base year inside chrono's range, and the
in-range invalid-date check uses chrono 0.4.45's `[-262143, 262142]`. Unit test
`extreme_year_chrono_bound_neighbours_do_not_panic` covers `262543-01-01`,
`-262544-01-01` and neighbours. The latest push also rustfmt's that
expected-micros chain (Lint was failing on the one-line `checked_mul` form) and
keeps the arithmetic probe.
(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:
Agreed on the helper — `offset_secs_at` is shared by the main path and the
extreme-year probe (with a checked 3h subtract so gap probing cannot panic).
The `None`/`Ambiguous` arms are kept for the rare case where a congruent civil
time falls in a DST gap/overlap; a second `None` still falls back to UTC (0). I
still do not have a constructed input that hits those arms for the extreme
probe itself.
(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:
`CometDateTimeUtilsSuite` now compares Comet to Spark via
`checkCastToTimestampAsLong` under `America/Los_Angeles` and `Asia/Tokyo` for
`-290308-12-21 19:59:05.224192`, `-262544-01-01`, `-262543-01-01`,
`-262500-01-01`, `262542-01-01`, and `262543-01-01` (Long seconds so the 422s
LMT-vs-PST gap survives truncation). The Rust unit test pins the micros values
for both eras; the duplicated UTC `i64::{MIN,MAX}` assertions stay in
`extreme_year_boundary_test` only. I could not run the Scala suite locally (box
has no Maven); relying on CI for the Spark comparison.
(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]
