ump45nose commented on code in PR #7255:
URL: https://github.com/apache/datafusion-comet/pull/7255#discussion_r4424116960
##########
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 for catching this — you are right that the 2000+rem far-future window
still sits inside temporary historical *standard* offsets, not just LMT.
Asia/Colombo is the concrete case: 262400 ≡ 2000 (mod 400), and 2000 was
still in the 1996–2006 +06:00 regime, while Java/Spark ZoneRules use the
permanent +05:30 past the last transition (1800s gap). That is separate from
the #6816 summer-DST horizon, and it did contradict the earlier "January
matches Spark" claim.
In be7d524 the far-future probe is now 2200..=2599 (still Gregorian-aligned
and inside chrono's range), so those temporary standards are skipped; the
far-past 400..=799 LMT window is unchanged. Added `262400-01-01` in
Asia/Colombo to the Rust extreme-year unit test and to
CometDateTimeUtilsSuite's 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]
