adriangb commented on PR #25163:
URL: https://github.com/apache/datafusion/pull/25163#issuecomment-5626585948
**Self-review: a QA pass on my own PR.** I checked every claim in the
description against a real PostgreSQL 17.11 and a real DuckDB 1.5.2, and I
re-ran the build and the tests. I found no correctness bug. The sign
convention, the return type and the `EXTRACT` path are all correct. Below are
one user-visible gap, three nits, and then the full list of what I checked.
---
## 1. `date_part('timezone', now())` fails out of the box
This is the first thing a user tries, and it does not work with the default
configuration:
```sql
> SELECT arrow_typeof(now());
Timestamp(ns)
> SELECT date_part('timezone', now());
Execution error: Date part 'timezone' is not supported for timezone-naive
timestamps, got Timestamp(ns)
```
`datafusion.execution.time_zone` defaults to `None`, so `now()` is
timezone-naive. A session zone fixes it:
```sql
> SET datafusion.execution.time_zone = 'Europe/Brussels';
> SELECT arrow_typeof(now()), date_part('timezone', now());
Timestamp(ns, "Europe/Brussels") 7200
```
PostgreSQL answers `date_part('timezone', now())` directly, because its
`now()` is always `timestamptz`.
The root cause is https://github.com/apache/datafusion/issues/25166, not
this PR, and the fix does not belong here. But the PR adds the exact function a
user reaches for, so the dead end is new. Two cheap additions cover it:
- a `.slt` case for `now()` with and without a session zone, which pins the
gap and turns into a diff when 25166 lands;
- one sentence in the `user_doc!` block that names
`datafusion.execution.time_zone`.
## 2. The doc sentence over-states the sign rule
The doc block says: "For a negative offset both parts carry the sign". That
is not exact. When the absolute offset is under one hour, the hour is `0` and
loses the sign, and only the minute carries it. The code is still right —
PostgreSQL does the same thing:
| engine | zone | instant | `timezone` | `timezone_hour` | `timezone_minute`
|
| --- | --- | --- | --- | --- | --- |
| PostgreSQL 17.11 | `Africa/Monrovia` | 1900-01-01T12:00:00Z | -2588 | 0 |
-43 |
| this PR | `Africa/Monrovia` | 1900-01-01T12:00:00Z | -2588 | 0 | -43 |
Truncation towards zero matches on the seconds too, so this is an exact
match and not a near miss. No modern zone has a negative offset under one hour,
so only historical LMT data reaches it.
Two suggestions:
- reword to "the minute carries the sign of the offset, and so does the hour
when the offset is a whole hour or more";
- add the `Africa/Monrovia` row to `timezone_parts_match_postgres`. It is
the one case the current 15-row table does not reach, and both engines agree on
it.
## 3. Move `to_lowercase` inside `TimezonePart::parse`
The call site is correct today, and I confirmed mixed case works after the
#24906 rebase. But the invariant is fragile:
```rust
Err(_) => match TimezonePart::parse(&part_trim.to_lowercase()) {
```
`part_normalization` strips quotes only; it does not lower the case.
`DatePart::from_str` and `is_epoch` each lower the case *inside* themselves.
`TimezonePart::parse` is the one function in the file that depends on its
caller. A second caller would silently break `date_part('TIMEZONE_HOUR', ...)`,
and no local test catches it. Move `.to_lowercase()` into `parse` so the type
owns its own contract.
## 4. Two error shapes for one condition
The two messages read as two different rules:
- `Date part 'timezone' is not supported for timezone-naive timestamps, got
Timestamp(ns)`
- `Date part 'timezone' is only supported for timestamps with a timezone,
got Date32`
Both mean "this value has no UTC offset". One shape is easier to grep for
and easier to document. Neither message tells the user what to do next. A hint
helps: name `AT TIME ZONE` or `datafusion.execution.time_zone`.
## 5. Two small errors in the PR description
- The count is `349 tests`; the real number is `353`. The lib suite goes
from 344 to 353.
- The "After this PR" block shows a column named `utc_offset_seconds`, but
the query carries no such alias.
I fix both in the rewritten description.
---
## What I checked, and what it measured
**Sign convention, against PostgreSQL 17.11.** `SET TimeZone TO '<zone>'`,
then `date_part('<part>', timestamptz '<instant>')`. All 10 rows of the table
in the PR description reproduce exactly, and 6 more zone and instant pairs do
too:
| zone | instant | `timezone` | `timezone_hour` | `timezone_minute` |
| --- | --- | --- | --- | --- |
| `America/St_Johns` | 2024-01-01T12:00:00Z | -12600 | -3 | -30 |
| `America/St_Johns` | 2024-07-01T12:00:00Z | -9000 | -2 | -30 |
| `Pacific/Chatham` | 2024-01-01T12:00:00Z | 49500 | 13 | 45 |
| `Pacific/Chatham` | 2024-07-01T12:00:00Z | 45900 | 12 | 45 |
| `Asia/Kolkata` | both | 19800 | 5 | 30 |
| `Asia/Kathmandu` | both | 20700 | 5 | 45 |
| `Europe/Brussels` | Jan / Jul | 3600 / 7200 | 1 / 2 | 0 / 0 |
| `America/Denver` | Jan / Jul | -25200 / -21600 | -7 / -6 | 0 / 0 |
| `Pacific/Marquesas` | both | -34200 | -9 | -30 |
| `UTC` | both | 0 | 0 | 0 |
So the negative minute is confirmed, not assumed.
**DuckDB 1.5.2 supports all three fields**, and it agrees with PostgreSQL on
every zone above. Two useful differences:
- DuckDB returns `BIGINT`, not `double precision`. So an integer return type
has precedent, and `Int32` here is not an outlier.
- DuckDB returns `0` for a timezone-naive timestamp; PostgreSQL raises `unit
"timezone" not supported for type timestamp without time zone`. This PR follows
PostgreSQL. That is the right call, but the description presents `0` as
self-evidently wrong when a major engine ships it. Worth a mention rather than
a silent omission.
DuckDB rejects `DATE` and `INTERVAL` input, as this PR does.
**Return type.** `return_field_from_args` gives `Float64` for `epoch`,
`Int64` for `nanosecond` and `Int32` for everything else. So the claim in the
description holds, and `Int32` is the consistent choice. The divergence from
PostgreSQL's `double precision` is pre-existing and applies to `hour`, `minute`
and the rest already. It does not need a call-out in this PR.
**`EXTRACT` needs no planner change.** Confirmed end to end. sqlparser 0.62
renders `DateTimeField::TimezoneHour` as `TIMEZONE_HOUR`, the planner forwards
that string, and the `.slt` cases for all three fields pass.
**Case insensitivity after the #24906 rebase.** `date_part('TIMEZONE',
...)`, `date_part('TIMEZONE_HOUR', ...)` and `date_part('Timezone_Minute',
...)` all work, in the unit tests and in the `.slt`. See nit 3 for the
fragility, not for a defect. Leading and trailing whitespace fails
(`date_part(' timezone ', ...)`), exactly as it fails for every other part on
`main`. Quoted spellings such as `'''timezone'''` work.
**Nullability.** `unary_opt` can turn an out-of-range value into a `NULL`
even when the input field is not nullable. `date_part('hour', ...)` on the same
value does the same thing, so Arrow's own kernel already behaves this way.
Pre-existing and consistent, so no action.
**`preimage`.** `DatePart::from_str("timezone")` fails, so `preimage`
returns `PreimageResult::None` and no filter rewrite fires. Constant folding
still works: `EXPLAIN` shows `Projection: Int32(7200)`.
**Both commits build green on their own.**
- commit 1 (`refactor: move parse_tz`): `cargo check --workspace
--all-targets` clean, `cargo clippy -p datafusion-functions --all-targets -- -D
warnings` clean, 344 lib tests pass.
- commit 2 (`feat: ... date_part`): `cargo clippy --all-targets -- -D
warnings` clean on the whole workspace, 353 lib tests pass,
`datetime/date_part.slt` green.
**Generated docs are in sync.** I re-ran `print_functions_docs -- scalar`
and diffed the `date_part` section against the committed file. The content
matches; the only differences are the prettier reflow that
`dev/update_function_docs.sh` applies. `./ci/scripts/doc_prettier_check.sh` and
`typos` are both clean.
**The doc example is real.** `SELECT date_part('timezone', TIMESTAMP
'2024-07-01T12:00:00' AT TIME ZONE 'Europe/Brussels')` returns `7200`, and the
input type is `Timestamp(ns, "Europe/Brussels")`.
**Merge order.** https://github.com/apache/datafusion/pull/25175 pins `Date
part 'timezone_hour' not supported` in its SECTION 9. Whichever PR merges
second must update the other. That PR already records the collision in its own
description.
🤖 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]
