comphead commented on code in PR #7219:
URL: https://github.com/apache/datafusion-comet/pull/7219#discussion_r4379461515
##########
native/spark-expr/src/utils.rs:
##########
@@ -206,6 +206,45 @@ pub(crate) fn resolve_local_datetime(tz: &Tz,
local_datetime: NaiveDateTime) ->
}
}
+/// Microseconds in one 400-year Gregorian cycle (146097 days). Calendars,
weekdays and
+/// therefore recurring time-zone rules repeat with this period.
+const MICROS_PER_GREGORIAN_CYCLE: i64 = 146_097 * 86_400 * 1_000_000;
+const MILLIS_PER_GREGORIAN_CYCLE: i64 = 146_097 * 86_400 * 1_000;
+
+/// How many Gregorian cycles a value outside chrono's year range
(-262144..=262143) is moved
+/// towards the epoch before its offset is looked up. 40,000 years brings
every `i64` microsecond
+/// value (years -290308 to 294247) into range.
+const OUT_OF_RANGE_SHIFT_CYCLES: i64 = 100;
Review Comment:
The 40,000-year shift covers the whole `i64` microsecond range, but
`local_to_utc` also serves the millisecond arm with
`MILLIS_PER_GREGORIAN_CYCLE`. `i64` milliseconds span about 292 million years,
so a millisecond value beyond roughly year 302,000 is still out of chrono's
range after the shift and keeps failing with `datetime_cast_err`. Could the
cycle count come from the value (enough cycles to land just inside chrono's
range), or could the doc say the millisecond path is only partly covered?
##########
native/spark-expr/src/utils.rs:
##########
@@ -427,6 +464,55 @@ mod tests {
);
}
+ fn ntz_micros(values: Vec<i64>) -> ArrayRef {
+ Arc::new(TimestampMicrosecondArray::from(values))
+ }
+
+ #[test]
+ fn test_timestamp_ntz_to_timestamp_handles_values_beyond_chrono_range() {
+ // Spark converts the whole i64 range; chrono stops at years -262144
and 262143.
+ let output =
+ timestamp_ntz_to_timestamp(ntz_micros(vec![i64::MAX, i64::MIN,
0]), "UTC", None)
+ .unwrap();
+ let output = as_primitive_array::<TimestampMicrosecondType>(&output);
+ assert_eq!(output.values(), &[i64::MAX, i64::MIN, 0]);
+
+ // A fixed offset is applied unchanged: +02:00 local is two hours
earlier in UTC.
+ let two_hours = 2 * 3_600 * 1_000_000;
+ let output =
+ timestamp_ntz_to_timestamp(ntz_micros(vec![i64::MAX]), "+02:00",
None).unwrap();
+ assert_eq!(
+ as_primitive_array::<TimestampMicrosecondType>(&output).value(0),
+ i64::MAX - two_hours
+ );
+ }
+
+ #[test]
+ fn
test_timestamp_ntz_to_timestamp_beyond_chrono_range_matches_shifted_offset() {
Review Comment:
This test derives its expected value from the same shifted lookup the code
does, so it cannot tell whether the result matches Spark.
`cast_timestamp_ntz.sql` already runs `cast(ts_ntz as timestamp)` under a
`ConfigMatrix` of UTC, `America/Los_Angeles`, `America/New_York` and
`Asia/Kolkata`. Rows a day inside each end of the range, for example built as
in the issue's `timestamp_micros` repro, would compare the far-past and
far-future results with Spark in all four zones without hitting `long overflow`
in any of them. That would also cover whether chrono-tz's far-future offsets
agree with `java.time`, and this test could then go. I have not run such a
fixture.
--
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]
