adriangb commented on PR #25175: URL: https://github.com/apache/datafusion/pull/25175#issuecomment-5636466035
**Self-review (QA pass) of our own PR, at head `bafa661263`.** I started this review at `a32121369b`. The head moved during the review, so I ran each check again at `bafa661263`. The main hazard of this file: a pinned bug without a comment reads as an intended contract. So I checked each expectation that records a bug or a divergence from PostgreSQL. For each one I asked two questions: does a comment name the problem, and does it link the specific issue? **Result.** The file passes at this head, alone and merged with current `main`. But CI shows a red license check. I also found pins with no comment, pins that link the wrong issue, and three issues that the file says it pins but no expectation exercises. The merge-order section of the PR body is correct for the five PRs that I checked. It does not name https://github.com/apache/datafusion/pull/25182, which causes the red license check. PostgreSQL answers below come from `postgres:15`. DuckDB answers come from DuckDB 1.5.2. DataFusion answers come from this file, or from `datafusion-cli` at the same base. ## Findings (most important first) ### 1. CI: "Check License Header" fails at this head - The job reports: `conflict datafusion/sqllogictest/test_files/datetime/timestamps_timezone.slt`. - Cause: https://github.com/apache/datafusion/pull/25182 (merged 2026-09-11 10:46 UTC) adds `*.slt` to `licenserc.toml`. The `.slt` headers on `main` have `#` on lines 8 and 10. Lines 8 and 10 of this file are empty. - The same job passed at `a32121369b` on 2026-09-10, before #25182 merged. Fix: put `#` on lines 8 and 10. https://github.com/apache/datafusion/pull/25164 has the same header and needs the same fix. ### 2. Section 8 links the wrong issue, and several of its pins have no comment The Section 8 header says: "This is the area corrupted by unwrap_cast", and links https://github.com/apache/datafusion/issues/25095. But no query in Section 8 has the shape of 25095. That issue is about a **naive column** compared with an **aware literal** under a non-UTC session time zone. Section 8 pins a different behaviour: a **naive literal** compared with an **aware column** is read in the time zone of the column, not in the session time zone. These expectations record that behaviour: | Lines | Query (short form) | DataFusion | PostgreSQL 15 | Comment in the file | | --- | --- | --- | --- | --- | | 534-537 | `cmp_denver WHERE ts = '2024-07-01 12:00:00'`, session unset / PG `UTC` | the `18:00Z` row | the `12:00Z` row | yes, no issue link | | 542-545 | `cmp_denver WHERE ts > '2024-07-01 06:00:00'` | 1 row | 2 rows | yes, no issue link | | 554-557 | `cmp_utc WHERE ts = '2024-07-01 12:00:00'`, session `America/Denver` | the `12:00Z` row | the `18:00Z` row | general note only, no link | | 578-582 | `c_denver: column1 = TIMESTAMP '2024-07-01 12:00:00'`, session unset / PG `UTC` | `false`, `true` | `f`, `f` | **none** | | 613-625, 641-645 | `EXPLAIN` of the same predicates | constant folded in the column zone | not applicable | "so that any optimizer change shows up"; no divergence marker, no link | No issue in our list covers this exactly. The closest open issue is https://github.com/apache/datafusion/issues/13212 ("`datafusion.execution.time_zone` is not used for basic time zone inference"). Link 13212 on each of these queries, or file a narrow issue and link that. Remove the unwrap_cast sentence from the header. ### 3. Two `KNOWN WRONG` pins link 25095, but the cause is 25166 - Lines 584-594: `cmp_denver WHERE ts = '2024-07-01T06:00:00Z'::timestamptz` returns the `12:00Z` row. PostgreSQL 15 returns the `06:00Z` row. - Lines 1214-1225: the `CASE` returns `2024-07-01T00:00:00-06:00`, which is `06:00Z`. PostgreSQL 15 returns `2024-07-01 00:00:00+00`. In both queries the session time zone is unset, so `'...Z'::timestamptz` gives a naive value (Section 0). The coercion then reads that wall clock in the Denver zone. The optimizer unwraps no cast on a column, so 25095 is not the cause. Link https://github.com/apache/datafusion/issues/25166 instead. ### 4. Lines 1191-1201 pin the wrong row, and the comment says the result is correct ```sql SELECT ts FROM cmp_denver WHERE ts = '2024-07-01T06:00:00Z'::timestamptz ``` This is the second `UNION ALL` branch. It is the same query as in finding 3, so it returns the `12:00Z` row. The expected output records `2024-07-01T12:00:00Z`. The comment says "The instants are preserved". PostgreSQL 15 returns the `06:00Z` row. Add a comment that names the wrong row and links https://github.com/apache/datafusion/issues/25166. Or set a session time zone for this query, so that the `UNION` test does not depend on this bug. ### 5. Three issues have no expectation that exercises them The PR says that a fix "shows up here as an exact diff". For these three issues, a fix does not change this file: - **https://github.com/apache/datafusion/issues/25095.** No query compares a naive column with an aware literal under a non-UTC session time zone. The bug is real at this base. Under `America/Denver`, `SELECT count(*) FROM naive_col WHERE column1 = '2024-07-01T18:00:00Z'::timestamptz` returns `0` in DataFusion. PostgreSQL 15 returns `1`. - **https://github.com/apache/datafusion/issues/12892.** Lines 835-842 run with the session time zone unset, so the result is the same before and after the fix. I merged https://github.com/apache/datafusion/pull/25161 (the fix) into this branch, and this file still passes. The bug is real at this base: under `America/Denver`, `from_unixtime(1719792000)` returns the naive `Timestamp(s)`. - **https://github.com/apache/datafusion/issues/25166, second part** (a cast to `timestamptz` replaces the time zone of an aware value). No query casts an aware value to `timestamptz`. The bug is real at this base: under `UTC`, a `Europe/Brussels` value cast to `::timestamptz` has the type `Timestamp(ns, "UTC")`. Add one query for each. ### 6. Other pins with no comment or no link - **Lines 339-350** (`KNOWN WRONG`, the composed `AT TIME ZONE` idiom): no issue link. Link https://github.com/apache/datafusion/issues/25166 and https://github.com/apache/datafusion/issues/12218. The comment also says "a whole day out". DataFusion returns `2024-07-02T00:00:00` and PostgreSQL 15 returns `2024-07-01 12:00:00`. The difference is 12 hours. - **Lines 364-370** (5c, `AT TIME ZONE '+05:30'` on a naive column): only a forward reference at line 303. DataFusion gives `12:00:00+05:30` (06:30Z). PostgreSQL 15 gives `2024-07-01 17:30:00+00`. Add a local link to https://github.com/apache/datafusion/issues/25170. - **Lines 452-463** (aware to naive to aware round trip): "same problem in the other direction", with no link. PostgreSQL 15 returns the input unchanged. Link https://github.com/apache/datafusion/issues/12218. - **Lines 716-722** (`date_bin` and `date_trunc` at `1 month`): the two functions disagree, and no comment says so. PostgreSQL 15 rejects the stride: `timestamps cannot be binned into intervals containing months or years`. Link https://github.com/apache/datafusion/issues/25167. - **Lines 1411-1420** (`dst_cross`): the comment notes that `date_bin` bins on the UTC instant, but does not link https://github.com/apache/datafusion/issues/25167. - **Lines 1139-1146 and 1471-1475** (`ts::timestamp` on a Denver column and on a Kolkata column): both show the UTC wall clock. This is the behaviour of 6b, which links https://github.com/apache/datafusion/issues/12218. These two do not. Under `TimeZone = 'Asia/Kolkata'`, PostgreSQL 15 returns `17:30:00`. DataFusion returns `12:00:00`. - **Lines 794-804** (`timezone_hour`, `timezone_minute`): the link goes to https://github.com/apache/datafusion/issues/10368, an umbrella with four separate requests. Link https://github.com/apache/datafusion/pull/25163, which implements exactly this. The comment says PostgreSQL returns "-6 and 0 for Denver in summer". That is true only under `TimeZone = 'America/Denver'`. Under `UTC`, PostgreSQL 15 returns `0` and `0` for the same instant. - **Value zone against session zone** (lines 768-792 `date_part`, 1027-1038 `168 days`, and the `date_trunc` half of 662-671): DataFusion uses the time zone of the value, PostgreSQL uses the session time zone. The comments describe this, but no issue tracks it. Under `TimeZone = 'UTC'`, PostgreSQL 15 gives hours `0, 6, 12, 18` for the `day_denver` rows. DataFusion gives `18, 0, 6, 12`. Say in the comment whether this is a bug or an intended design, and link an issue if it is a bug. - **Minor type notes with no link**: lines 865-892 (`to_timestamp*` naive), 992-997 (`now()` naive), 1069-1079 (`'...Z'::timestamptz + INTERVAL` naive) and 1227-1237 (`CASE` naive). All four follow from https://github.com/apache/datafusion/issues/25166 with the session time zone unset. One link per block is enough. ### 7. Lines 733-742 call the `date_bin` origin drift a bug, but PostgreSQL and DuckDB drift the same way For the two `dst_origin` rows under `America/Denver`: | Engine | Bin for `2024-03-11T12:30:00Z` | | --- | --- | | DataFusion (this file) | `2024-03-11T01:00:00-06:00` | | PostgreSQL 15, `date_bin` | `2024-03-11 01:00:00-06` | | DuckDB 1.5.2, `time_bucket` | `2024-03-11 01:00:00-06` | The pin is correct, and https://github.com/apache/datafusion/issues/25168 is the right link. But the comment must say that PostgreSQL and DuckDB give the same answer. Otherwise a fix can claim PostgreSQL parity that does not exist. ### 8. Low-priority notes - The header "Related issues" list does not include https://github.com/apache/datafusion/issues/25168. - Line 1181 says: "the naive side is treated as UTC". That is not true in general. I joined `cmp_denver` with `naive_col`, with the session time zone unset. The naive `2024-07-01 12:00:00` matches `2024-07-01T12:00:00-06:00`, which is 18:00 UTC. So the naive side is read in the zone of the column. PostgreSQL 15 under `UTC` matches the 12:00 UTC row. - The PR body says that the 40 `SET datafusion.execution.time_zone` statements on `main` are "nearly all" in two files. On `main` they are in 9 files. `set_variable.slt` has 1 of them. - https://github.com/apache/datafusion/pull/25171 adds docs that say DataFusion "discards the `Z`" in `'...Z'::timestamptz`. Lines 52-56 of this file say that the offset "is still honoured". Lines 90-95 confirm this: `+05:30` becomes `06:30:00`. The two texts must agree. The offset is applied, and only the time zone label is lost. ## Merge-order collisions, checked against the file I merged each PR into this branch at `bafa661263` and ran this file, except where the diff shows no possible effect. | PR (head) | Change | Result of the merged run | Expectations that change | | --- | --- | --- | --- | | https://github.com/apache/datafusion/pull/25165 (`3e42f8b102`) | `AT TIME ZONE` on an aware value returns a naive value | 2 failures | lines 323 and 331 (5b): `Timestamp(ns, "Europe/Brussels") 2024-01-15T13:00:00+01:00` becomes `Timestamp(ns) 2024-01-15T13:00:00`, and the Denver query changes the same way | | https://github.com/apache/datafusion/pull/25163 (`bdbfe1f8cd`) | `date_part` accepts `timezone`, `timezone_hour` and `timezone_minute` | 2 failures | lines 800 and 803: the two `query error` blocks now succeed | | https://github.com/apache/datafusion/pull/25161 (`fb72121aef`) | `from_unixtime` uses the session time zone | pass | none. The comment at lines 835-838 becomes stale | | https://github.com/apache/datafusion/pull/25173 (`aeb4122371`) | `generate_series` precision | not run | none: this file has no `generate_series`, and the PR changes only `generate_series.rs` and `table_functions.slt` | | https://github.com/apache/datafusion/pull/25171 (`095e6d3a31`) | a code comment and docs | not run | none: no behaviour change. See the `Z` note in finding 8 | | current `main` | includes https://github.com/apache/datafusion/pull/24920 and https://github.com/apache/datafusion/pull/25182 | pass | none. At `a32121369b`, #24920 broke at least 10 Section 16 error expectations. `bafa661263` fixes them | The updated https://github.com/apache/datafusion/pull/25165 adds a `CASE` limitation test in `timestamps.slt`. This file applies `AT TIME ZONE` to no `CASE` expression, so that test adds no collision. ## What I checked and found correct - The file passes at `bafa661263` (`cargo test --profile ci -p datafusion-sqllogictest --test sqllogictests -- timestamps_timezone`), and also with current `main` merged in. - These comments name the problem and link the right issue: Sections 0-4 (https://github.com/apache/datafusion/issues/25166), the `'+05:30'` literal in 5a (https://github.com/apache/datafusion/issues/25170), 5b, 6b and the first round trip in 7 (https://github.com/apache/datafusion/issues/12218), the Denver and Kolkata `date_bin`/`date_trunc` cases (https://github.com/apache/datafusion/issues/25167), the origin drift (https://github.com/apache/datafusion/issues/25168), `from_unixtime` (https://github.com/apache/datafusion/issues/12892), and all of Section 16 (https://github.com/apache/datafusion/issues/25084). - I measured the PostgreSQL answers that the comments quote. They are correct for 5a, 5b, the composed idiom, 6b, the round trips, the Section 8 row counts, the `168 days` case, `to_timestamp`, `current_time`, the `UNION` type, and all four Section 16 DST values. - The new Section 16 expectations match the stable `Arrow error: ...` tail, so they still separate the column path (`Cannot cast timezone to different timezone`) from the literal path (`error computing timezone offset`). 🤖 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]
