adriangb opened a new pull request, #25175: URL: https://github.com/apache/datafusion/pull/25175
## Which issue does this PR close? This PR closes no issue and fixes no bug. It adds regression-prevention coverage for the area the issues below live in, and records the current (in several places wrong) behaviour so that a fix for any of them shows up here as an exact diff. - https://github.com/apache/datafusion/issues/10368 - https://github.com/apache/datafusion/issues/10602 - https://github.com/apache/datafusion/issues/12218 - https://github.com/apache/datafusion/issues/12892 - https://github.com/apache/datafusion/issues/13212 - https://github.com/apache/datafusion/issues/25084 - https://github.com/apache/datafusion/issues/25095 - https://github.com/apache/datafusion/issues/25166 - https://github.com/apache/datafusion/issues/25167 - https://github.com/apache/datafusion/issues/25170 ## Rationale for this change DataFusion has a long tail of timezone correctness bugs. They keep recurring, and the reason is structural: almost nothing in the test suite pins the behaviour down, so a change to timezone semantics can land without producing a single test diff. Concretely, on today's `main`: - `SET datafusion.execution.time_zone` appears roughly 40 times in the entire sqllogictest corpus, and nearly all of those are concentrated in two files (`to_timestamp_timezone.slt` and `set_variable.slt`). - DST-boundary dates are essentially absent. Nothing exercises an ambiguous local time (a wall clock that occurs twice) or a non-existent one (a wall clock inside the spring-forward gap). The result is that the semantics people actually hit in production — what `::timestamptz` produces, whether a naive literal in a `WHERE` is read in the session zone or the column's zone, whether `date_bin` and `date_trunc` agree, what happens at 01:30 on the first Sunday in November — are undefined by the test suite. A refactor can change any of them silently, and a fix for one can regress another. This PR does not change any behaviour. It writes the current behaviour down, so that from now on **any change to timezone semantics shows up as a test diff** and has to be argued for rather than discovered by a user. Where today's behaviour is wrong, or disagrees with PostgreSQL, the expected output still records what DataFusion does — with a comment directly above saying so and linking the issue. When a fix lands, the diff is the specification of what changed. This is the companion to https://github.com/apache/datafusion/pull/25164, which adds the first `pg_compat` file for `timestamptz` and contains only the subset the two engines agree on. The two were split apart because they have very different risk profiles: a failure in the pg_compat file is always a real regression, whereas this file deliberately pins behaviour we expect to change. ## What changes are included in this PR? One new test file. No production code is touched. **`datafusion/sqllogictest/test_files/datetime/timestamps_timezone.slt`** (~1550 lines, 18 labelled sections) Every case is exercised both as a literal and, where it matters, as a real CTAS column — several of these behaviours only reproduce on columns. - **0–4** — `arrow_typeof` and value of `'...'::timestamp`, `'...'::timestamptz`, `TIMESTAMP '...'` and `TIMESTAMP WITH TIME ZONE '...'` under session zone unset / `+00:00` / `+05:30` / `America/Denver` / `Europe/Brussels`, for literals and for columns. Records that with the session zone **unset**, `::timestamptz` yields a tz-**naive** `Timestamp(ns)` (https://github.com/apache/datafusion/issues/25166). - **5** — `AT TIME ZONE` on tz-naive and tz-aware input, type and value, including the fixed-offset-string sign convention (https://github.com/apache/datafusion/issues/25170). - **6** — casts in all four directions: naive→named, named→naive, named→other named, naive→fixed offset, named→fixed offset. - **7** — round trips, including `t = t::timestamptz::timestamp` under UTC and non-UTC session zones. - **8** — comparison and equality between a tz-aware column and a tz-naive literal and vice versa, with `EXPLAIN` of the filter so that any change to the `unwrap_cast` / `simplify_expressions` rewrite is visible in the plan (https://github.com/apache/datafusion/issues/25095). - **9–11** — `date_bin`, `date_trunc`, `date_part`, `extract`, `to_char`, `from_unixtime`, `to_unixtime`, `to_timestamp*`, `to_local_time`, `now`, `current_date`, `current_time`, `make_date` on tz-aware input. - **12** — timestamp ± interval across both DST transitions, and the `INTERVAL '1 day'` vs `INTERVAL '24 hours'` distinction in both directions. - **13–15** — `MIN`/`MAX`/`GROUP BY`/`ORDER BY`/`DISTINCT` over a tz-aware column; joins on tz-aware keys including a mixed `+00:00` / `America/Denver` join; `UNION`/`CASE`/`COALESCE`/`greatest` coercion across mixed zones. - **16** — the DST boundaries specifically: US fall-back `2024-11-03 01:30:00` and spring-forward `2024-03-10 02:30:00` in `America/Denver`, EU `2024-10-27 02:30:00` and `2024-03-31 02:30:00` in `Europe/Brussels`, each via column cast, `AT TIME ZONE`, string literal and `::timestamptz`. - **17** — a zone with no DST (`America/Phoenix`) and a half-hour zone (`Asia/Kolkata`). ## ⚠️ Merge-order conflict with #25165 **https://github.com/apache/datafusion/pull/25165 changes behaviour that SECTION 5 of this file pins. Whichever of the two merges second has to update the other, and CI on the second one will go red until it does.** #25165 makes `AT TIME ZONE` on an already timezone-**aware** input return a naive `Timestamp(ns)` (matching PostgreSQL) instead of a relabelled tz-aware value. SECTION 5b is exactly that case, on the `c_utc` and `c_denver` columns. Once #25165 lands, these two queries change: ```sql query TP rowsort SELECT arrow_typeof(column1 AT TIME ZONE 'Europe/Brussels'), column1 AT TIME ZONE 'Europe/Brussels' FROM c_utc ``` | | today (pinned here) | after #25165 | |---|---|---| | | `Timestamp(ns, "Europe/Brussels") 2024-01-15T13:00:00+01:00` | `Timestamp(ns) 2024-01-15T13:00:00` | | | `Timestamp(ns, "Europe/Brussels") 2024-07-01T14:00:00+02:00` | `Timestamp(ns) 2024-07-01T14:00:00` | ```sql query TP rowsort SELECT arrow_typeof(column1 AT TIME ZONE 'America/Denver'), column1 AT TIME ZONE 'America/Denver' FROM c_denver ``` | | today (pinned here) | after #25165 | |---|---|---| | | `Timestamp(ns, "America/Denver") 2024-01-15T12:00:00-07:00` | `Timestamp(ns) 2024-01-15T12:00:00` | | | `Timestamp(ns, "America/Denver") 2024-07-01T12:00:00-06:00` | `Timestamp(ns) 2024-07-01T12:00:00` | The `DIVERGES FROM POSTGRESQL (type, not instant)` comment above them should be deleted at the same time, since #25165 removes that divergence. Nothing else in the file collides. In particular the other `AT TIME ZONE` uses — 5a, 5c, the composed `(… ::timestamptz AT TIME ZONE …)::timestamp` case, the interval-arithmetic case in SECTION 12 and all of SECTION 16 — apply `AT TIME ZONE` to a timezone-**naive** input, which #25165 leaves alone. https://github.com/apache/datafusion/pull/25173 (`generate_series` timestamp precision) does not collide either: the file uses no `generate_series`. I have left the file pinning today's behaviour, since that is what this PR is for. Happy to rebase it onto #25165 instead if that one is closer to merging. ## What is the testing strategy for this PR? This PR *is* tests. Results were generated with the sqllogictest `--complete` mode rather than hand-written, and then every generated result was read and sanity-checked; several were wrong and were rewritten (e.g. queries that tripped DataFusion's projection-name-uniqueness rule, and a `- INTERVAL '1 day'` case that did not actually cross a DST boundary). Cross-engine claims in the comments were checked against a real PostgreSQL. Verified locally: - `cargo test -p datafusion-sqllogictest --test sqllogictests` — full suite green (506 files) - `cargo fmt --all`, `typos datafusion/ docs/` — clean ### Divergences from PostgreSQL recorded in the file Every one is recorded with both answers and a comment; none is fixed here. 1. **`::timestamptz` with the session zone unset is tz-naive.** PostgreSQL always yields `timestamp with time zone`. (https://github.com/apache/datafusion/issues/25166) 2. **`timestamptz AT TIME ZONE zone` returns a tz-aware value**, relabelled into `zone`; PostgreSQL drops the zone and returns a naive `timestamp` holding the wall clock in `zone`. This is what #25165 fixes. 3. **`AT TIME ZONE '+05:30'` uses the opposite sign convention from PostgreSQL's string form.** DataFusion reads `'+05:30'` as UTC+05:30; PostgreSQL reads the *string* POSIX-style (west positive) and only its `INTERVAL` form agrees with DataFusion. (https://github.com/apache/datafusion/issues/25170) 4. **`timestamptz::timestamp` always renders in UTC** and ignores `datafusion.execution.time_zone`; PostgreSQL renders in the session `TimeZone`. (https://github.com/apache/datafusion/issues/12218) 5. **The naive↔aware round trip is not the identity** under a non-UTC session zone, as a consequence of 4. 6. **A tz-naive literal compared against a tz-aware column is coerced into the column's zone**, not the session zone. `SELECT ts FROM t WHERE ts > '2024-07-01 06:00:00'` over a Denver-typed column returns 1 row in DataFusion and 2 in PostgreSQL under `TimeZone='UTC'`. The session zone has no effect on this path at all. (https://github.com/apache/datafusion/issues/25095) 7. **A tz-aware literal loses its offset when the session zone is unset.** `WHERE ts = '2024-07-01T06:00:00Z'::timestamptz` against a Denver column returns the **12:00Z** row, not the 06:00Z one — the `Z` is silently discarded and the wall clock re-read in the column's zone. Setting a session zone fixes it. Same root cause in `CASE`. 8. **`date_trunc` truncates in the value's zone**; PostgreSQL truncates in the session `TimeZone` (or an explicit third argument, which DataFusion does not accept). `date_bin`, by contrast, agrees with PostgreSQL. 9. **`date_bin` and `date_trunc` disagree with each other inside DataFusion** for the same input, whenever the zone offset is not a whole multiple of the stride. (https://github.com/apache/datafusion/issues/25167) 10. **`date_part('timezone_hour')` and `date_part('timezone_minute')` are rejected** — `Execution error: Date part 'timezone_hour' not supported`. PostgreSQL supports both. There is currently no way to ask a tz-aware value for its own offset. (part of https://github.com/apache/datafusion/issues/10368) 11. **Day-valued interval arithmetic is DST-aware in the value's zone**, whereas PostgreSQL scopes it to the session `TimeZone`. When the two are aligned the engines agree exactly, and that agreement is pinned in #25164. 12. **Ambiguous and non-existent local times fail outright** (see below). 13. Minor type-level differences, recorded for completeness: `now()` is tz-naive with the session zone unset, `current_time` is `Time64(ns)` where PostgreSQL has `time with time zone`, and `from_unixtime`/`to_timestamp*` return naive values unless a session zone is set (https://github.com/apache/datafusion/issues/12892). ### The DST-boundary behaviour, precisely DataFusion cannot represent an ambiguous or non-existent local time in a named zone **by any route**. Column cast, `AT TIME ZONE`, string literal and `::timestamptz` under a session zone all error. The column path and the literal path fail with *different* errors for the same value: - column path — `Arrow error: Cast error: Cannot cast timezone to different timezone` - literal path — folded by `simplify_expressions`, then `Arrow error: Parser error: ... error computing timezone offset` PostgreSQL resolves all four cases: `2024-11-03 01:30:00` Denver → `01:30:00-07` (the second, standard-time occurrence); `2024-03-10 02:30:00` Denver → `03:30:00-06` (shifted forward out of the gap); `2024-10-27 02:30:00` Brussels → `02:30:00+01`; `2024-03-31 02:30:00` Brussels → `03:30:00+02`. This is https://github.com/apache/datafusion/issues/25084, and https://github.com/apache/arrow-rs/pull/11038 is the arrow-rs fix. Note that the failure is **not** limited to columns — literals fail too, just with a different message. Both paths, and both error texts, are pinned in the file so the fix will show up as an exact diff. Once the values are built from explicit UTC instants the boundary is navigable, and that behaviour is characterized too: both occurrences of 01:30 render with the correct differing offsets, `to_local_time` collapses them onto the same naive value, and `+ INTERVAL '1 hour'` walks through the repeated hour rather than over it. ## Are there any user-facing changes? No. This PR adds a single test file. No production code, no public API, and no behaviour is changed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
