sunchao commented on code in PR #7255:
URL: https://github.com/apache/datafusion-comet/pull/7255#discussion_r4426158539
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -1443,18 +1469,33 @@ 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).
+ // Far-past and far-future need different eras: a single 1600..=1999
window can
+ // land after a zone's LMT→standard transition (e.g. LA 1883, Tokyo
~1888) for
+ // some far-past residues, and still in the LMT era for far-future
residues
+ // (262543 → 1743), disagreeing with Spark/Java which use the first
offset before
+ // any transition and the permanent/final standard offset for
post-horizon dates.
+ // far-past → 400..=799 (before every IANA transition we care
about → LMT)
+ // far-future → 2200..=2599 (past temporary historical
standard-offset regimes
+ // such as Asia/Colombo +06:00 in 1996–2006; a 2000+rem
probe for
+ // year 262400 landed on 2000 and was 1800s early vs
Spark/Java
+ // +05:30. Summer DST after chrono-tz's last timespan
remains #6816)
+ // Fixed offsets and UTC are unchanged either way.
+ let rem = year.rem_euclid(400);
+ let probe_year = if year > CHRONO_MAX_YEAR {
+ (2200 + rem) as i32
Review Comment:
For `262400-02-29 00:00:00`, this expression produces the probe date
`2200-02-29`. Year 262400 is a leap year and 2200 is not, so `from_ymd_opt`
returns `None` and the `expect` below panics even in UTC. Spark 4.1.3 returns
`8218382083200000000` microseconds for that input in UTC. Can we use a future
base divisible by 400, such as `2400 + rem`, and cover this leap day alongside
the January Colombo case? That keeps the probe beyond Colombo’s historical
offset change and preserves the Gregorian cycle.
--
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]
