andygrove opened a new pull request, #6354:
URL: https://github.com/apache/datafusion-comet/pull/6354
## Which issue does this PR close?
Closes #5633.
## Rationale for this change
The timezone-aware path of the native `date_trunc` built the truncated time
with chrono's `with_hour`, `with_minute`, `with_day0` and `with_month0` on a
`DateTime<Tz>`. Those return `None` when the local result is ambiguous
(fall-back) or falls in a gap (spring-forward). `as_micros_from_unix_epoch_utc`
then unwrapped the `None`, so truncating a timestamp near a DST transition
panicked. `trunc_timestamp_dst_ambiguous.sql` had every query marked `ignore`
because of this.
Even where it didn't panic, the old path didn't follow Spark for ambiguous
results. Spark's `DateTimeUtils.truncTimestamp` treats the levels differently:
| Level | Spark | Ambiguous local result | Local result in a gap |
| --- | --- | --- | --- |
| `MICROSECOND`, `MILLISECOND`, `SECOND` | truncates the instant (offsets
are whole seconds) | n/a | n/a |
| `MINUTE`, `HOUR`, `DAY` | `ZonedDateTime.truncatedTo` | keeps the input's
offset if it is still valid | moved forward by the gap's length |
| `WEEK`, `MONTH`, `QUARTER`, `YEAR` | truncate the local date, then
`LocalDate.atStartOfDay` | earlier offset | first instant after the gap |
So on the 2024-11-03 fall-back in `America/Los_Angeles`, the second 01:30
(PST) truncates to 01:00 PST, not to the first 01:00 (PDT).
## What changes are included in this PR?
- The timezone-aware truncation now works on the local time. It truncates
the naive local datetime with the same helpers the `TIMESTAMP_NTZ` path uses,
then resolves it back to an instant with the rule for its level from the table
above. A gap takes the offset from before the gap through
`resolve_local_datetime`, the helper the date-to-timestamp cast already uses.
That gives the same instant as Java's rule.
- `MICROSECOND`, `MILLISECOND` and `SECOND` truncate the instant directly,
as Spark does, instead of going through the calendar.
- `as_micros_from_unix_epoch_utc` and the `DateTime<Tz>`-based helpers are
removed.
- A new Rust unit test checks 49 truncations against values computed with
`java.time`, which is what Spark's `truncTimestamp` calls. It covers both
01:30s of the 2024-11-03 Los Angeles fall-back, the 2024-03-10 spring-forward,
the 1970-10-25 repro from the issue, São Paulo's skipped midnight on
2018-11-04, and both 23:30s of its 2019-02-16 fall-back.
- `trunc_timestamp_dst_ambiguous.sql` runs its six queries again. It adds
the second occurrence of the ambiguous hour and a `MINUTE` query.
- A new `trunc_timestamp_dst_midnight.sql` covers São Paulo's midnight
transitions.
This conflicts with #5956, which also reworks this kernel. Truncation in
non-UTC sessions still defaults to the codegen dispatcher, since it remains
`Incompatible` because of the chrono-tz horizon and #6331. This makes the
opt-in native path correct.
@coderfender, I picked this up as part of the timezone EPIC (#6335). Happy
to hand it back if you already have work in progress.
## How are these changes tested?
- **Old kernel:** the new unit test panics at `temporal.rs:166`. Both
`trunc_timestamp_dst_ambiguous.sql` and `trunc_timestamp_dst_midnight.sql` fail
with the native panic.
- **With this change:**
- Both SQL files match Spark.
- All `expressions/datetime/` SQL file tests and
`CometTemporalExpressionSuite` pass on Spark 4.1.
- The new and existing kernel unit tests pass.
- `cargo clippy --all-targets -- -D warnings` is clean for
`datafusion-comet-spark-expr`.
--
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]
