comphead commented on code in PR #7255:
URL: https://github.com/apache/datafusion-comet/pull/7255#discussion_r4391176419
##########
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:
I could not construct an input that reaches the `LocalResult::None` or
`Ambiguous` arms for this probe, since it lands either before every transition
of the zone or after the last `chrono-tz` timespan, and neither arm has a test.
They also repeat the three-way match from the main path above, so a small
helper such as `offset_secs_at(tz, naive) -> Option<i64>` could serve both. In
the gap arm, `probe_naive - chrono::Duration::hours(3)` panics if the
subtraction overflows `NaiveDateTime`, and the final `LocalResult::None => 0`
silently treats the zone as UTC. Both are narrow and unverified, so this is a
question about whether the arms are needed.
##########
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:
The expected values in `extreme_year_session_tz_uses_historic_offset` are
asserted from the author's reading of Spark and are not compared against it.
`CometDateTimeUtilsSuite.checkCastToTimestampAsLong` already casts to `LONG` to
avoid the `toJavaTimestamp` limits, and the SQL harness supports `--
ConfigMatrix: spark.sql.session.timeZone=...`. Could one case there use
`America/Los_Angeles` and a positive-offset zone such as `Asia/Tokyo` with
`-290308-12-21 19:59:05.224192` and `-262544-01-01 00:00:00`, where the second
also exercises the point above? A 422 second difference survives the truncation
in `cast(ts as bigint)`. The Rust test also repeats the UTC `i64::MAX` and
`i64::MIN` assertions of `extreme_year_boundary_test`, and the 1800-06-01 case
is outside the extreme branch. I did not verify which session zone the Scala
suite runs under.
##########
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:
The loop bounds look one year wider than chrono accepts. In chrono 0.4.45,
which `Cargo.lock` pins, `MIN_YEAR` is `(i32::MIN >> 13) + 1 = -262143` and
`MAX_YEAR` is `(i32::MAX >> 13) - 1 = 262142`, and `NaiveDate::MAX` is
documented as December 31, 262142 CE. A year of `262543` is shifted once to
`262143` and the loop stops there, and `-262544` is shifted once to `-262144`
and stops there. `from_ymd_opt` then returns `None` and the `.expect` panics.
Both years pass the `-290309..=294248` gate and fit in `i64` microseconds, so
as far as I can tell `cast('262543-01-01' as timestamp)` returns a value in
Spark and on main (the old probe used 1970-01-01), but would panic here, under
UTC and fixed-offset sessions too, since this branch runs for every zone. Could
the bounds be `-262143` and `262142`, or could the two loops become arithmetic
with `rem_euclid(400)` and a base year inside the range, which also removes up
to about 80 iterations per value? Please add `262543-01-01`, `-262544-
01-01` and their neighbours to the unit test. The `(-262144..=262143)` check
at line 1436 has the same off-by-one and is outside this diff, so years 262143
and -262144 return `Ok(None)` as invalid dates. I derived the panic by reading
and did not run it.
--
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]
