Conversation
The timezone-aware date_trunc path built the truncated time with chrono's with_hour/with_minute/with_day0 on DateTime<Tz>, which return None when the local result is ambiguous or falls in a gap, and the kernel unwrapped that None and panicked. Truncate the local time instead and resolve it the way Spark's DateTimeUtils.truncTimestamp does: MINUTE, HOUR and DAY keep the input's offset when the result is ambiguous, as ZonedDateTime.truncatedTo does, the date levels use the earlier offset, as LocalDate.atStartOfDay does, and SECOND and finer truncate the instant. Closes apache#5633.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #5633.
Rationale for this change
The timezone-aware path of the native
date_truncbuilt the truncated time with chrono'swith_hour,with_minute,with_day0andwith_month0on aDateTime<Tz>. Those returnNonewhen the local result is ambiguous (fall-back) or falls in a gap (spring-forward).as_micros_from_unix_epoch_utcthen unwrapped theNone, so truncating a timestamp near a DST transition panicked.trunc_timestamp_dst_ambiguous.sqlhad every query markedignorebecause of this.Even where it didn't panic, the old path didn't follow Spark for ambiguous results. Spark's
DateTimeUtils.truncTimestamptreats the levels differently:MICROSECOND,MILLISECOND,SECONDMINUTE,HOUR,DAYZonedDateTime.truncatedToWEEK,MONTH,QUARTER,YEARLocalDate.atStartOfDaySo 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?
TIMESTAMP_NTZpath 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 throughresolve_local_datetime, the helper the date-to-timestamp cast already uses. That gives the same instant as Java's rule.MICROSECOND,MILLISECONDandSECONDtruncate the instant directly, as Spark does, instead of going through the calendar.as_micros_from_unix_epoch_utcand theDateTime<Tz>-based helpers are removed.java.time, which is what Spark'struncTimestampcalls. 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.sqlruns its six queries again. It adds the second occurrence of the ambiguous hour and aMINUTEquery.trunc_timestamp_dst_midnight.sqlcovers São Paulo's midnight transitions.This conflicts with #5956, which also reworks this kernel.
The serde still marks truncation in non-UTC sessions
Incompatible, so by default it still runs through the codegen dispatcher. This PR fixes the native path thatallowIncompatibleopts into. TheIncompatiblereason still cites #2649, which #4761 closed. Two known divergences remain: chrono-tz's DST rules end in 2099, which the datetime compatibility guide documents, and tzdata can drift from the JVM's (#6331). So I left the support level alone.@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?
temporal.rs:166. Bothtrunc_timestamp_dst_ambiguous.sqlandtrunc_timestamp_dst_midnight.sqlfail with the native panic.expressions/datetime/SQL file tests andCometTemporalExpressionSuitepass on Spark 4.1.cargo clippy --all-targets -- -D warningsis clean fordatafusion-comet-spark-expr.