ninglinLiu commented on code in PR #7219:
URL: https://github.com/apache/datafusion-comet/pull/7219#discussion_r4379636991
##########
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:
Changed in 0555805. The cycle count now comes from the value:
`shift_towards_epoch` moves it by whole 400-year cycles until it is 600 cycles
(240,000 years) from the epoch on the same side, which is inside chrono's range
for both units, so the millisecond arm covers the whole `i64` range too. The
shift is at most `|value|`, so it cannot overflow. New unit tests check
`i64::MAX`/`i64::MIN`/`0` milliseconds in UTC and `+02:00` (they fail with the
old fixed 100-cycle shift), and that the shifted value lands in range for both
units at both ends.
##########
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:
Done in 0555805: dropped the shifted-offset unit test and added a
`test_ntz_extreme` table to `cast_timestamp_ntz.sql` with rows a day inside
each end of the range plus January/July far-past and a January far-future row,
compared with Spark under the existing ConfigMatrix. Locally all four zones
pass. Two things I found while writing it:
- The result is read back as `cast(... as long)`: collecting the timestamp
itself fails in Spark's own `toJavaTimestamp` with `long overflow`, and
`cast(... as string)` on these values fails natively with the same chrono range
error (a separate gap in timestamp-to-string).
- A July far-future row differs from Spark in America/Los_Angeles and
America/New_York by one hour. This is not specific to the shift: chrono-tz
stops applying DST after 2099, so Comet already returns standard time for an
in-range `2100-07-15 12:00` in Los Angeles while `java.time` gives `-07:00`. I
left that row out with a comment in the fixture. Happy to open an issue for
both gaps if that is useful.
--
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]
