sunchao commented on code in PR #7255:
URL: https://github.com/apache/datafusion-comet/pull/7255#discussion_r4403170821
##########
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:
Could we choose the offset era separately for far-past and far-future dates?
`1600 + year.rem_euclid(400)` preserves the calendar but not timezone rules.
For `262543-01-01 00:00:00` it probes 1743: the pinned chrono-tz returns Los
Angeles offset -07:52:58 while Java/Spark uses -08:00, making the result 422
seconds early. Tokyo similarly returns +09:18:59 instead of +09:00, a
1,139-second error. The previous 1970 probe was correct for both January cases,
so this is a new regression, separate from the known future-DST horizon issue
#6816. In the other direction, `-262500-01-01` probes 1900 and still misses the
ancient LMT offset. I reproduced these offsets with the exact Rust dependencies
and Java. Please cover both future January and negative-year residues that land
after timezone transitions, and run the new Spark comparison test after
correcting the probe. A native-cast assertion or `expect_native(cast)` SQL
fixture would also ensure the regression test exercises this Rust path.
--
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]
