andygrove commented on PR #5956: URL: https://github.com/apache/datafusion-comet/pull/5956#issuecomment-5876604407
This is a light fully automated review since there are so many PRs open. With a non-UTC session timezone and `allowIncompatible` on, `minute` now always takes DataFusion's fast path (`native/spark-expr/src/kernels/temporal.rs:807`), which floors the UTC microseconds to a whole minute without looking at the zone. Spark truncates MINUTE in local time (`truncToUnit(micros, zoneId, ChronoUnit.MINUTES)` in `DateTimeUtils.truncTimestamp`), as did the chrono kernel this replaces. The two only agree while the zone's offset is a whole number of minutes. `Africa/Monrovia` was `-00:44:30` until 1972, so a `ts` of `1960-06-15 10:30:45` truncates to `10:30:00` in Spark but `10:30:30` here. `America/Los_Angeles` before November 1883 (`-07:52:58`) gives `10:30:02` instead of `10:30:00`. This is behind `allowIncompatible`, but it is a regression from the current kernel. Could `minute` stay zone-aware when the array carries a non-UTC zone, for example by subtracting `(micros + offset_micros).rem_euclid(60_000_000)` with the offset at that instant? That would also avoid falling back to the chrono kernel, which panics for values inside a DST overlap. Adding `Africa/Monrovia` and a 1960 row to `trunc_timestamp_dst_ambiguous.sql` would cover 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]
