sunchao commented on code in PR #7255:
URL: https://github.com/apache/datafusion-comet/pull/7255#discussion_r4414325366
##########
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:
Rechecked `7ff16366a54d9c90ea4de70208057b3b3a1ba71a`. The earlier LA/Tokyo
examples are fixed, but the future window still contains historical standard
offsets. A concrete non-DST case is `262400-01-01 00:00:00` in `Asia/Colombo`:
| Lookup | Offset |
| --- | --- |
| Current `2000 + year.rem_euclid(400)` probe, year 2000 | +06:00 (21,600
seconds) |
| Java `LocalDateTime.of(262400, 1, 1, 0,
0).atZone(ZoneId.of("Asia/Colombo"))` | +05:30 (19,800 seconds) |
| Previous year-1970 probe | +05:30 (19,800 seconds) |
I reproduced the probe with the pinned chrono 0.4.45 and chrono-tz 0.10.4
and compared it with Java 17. Subtracting the current probe offset makes the
timestamp 1,800 seconds early. This is separate from #6816's summer-DST
horizon, and it contradicts the new comment that January matches Spark.
Could the future-offset choice avoid historical standard-offset regimes as
well, with this case added to the Spark comparison and native cast execution
asserted? These are component-level results; I did not run the full Spark/Comet
suite.
--
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]
