adriangb opened a new pull request, #25165: URL: https://github.com/apache/datafusion/pull/25165
## Which issue does this PR close? - Closes https://github.com/apache/datafusion/issues/12218 ## Rationale for this change `AT TIME ZONE` is asymmetric in PostgreSQL, and DataFusion only implements one of the two halves. - `<timezone-naive timestamp> AT TIME ZONE zone` reads the value as a wall clock in `zone` and returns the corresponding **timezone-aware** instant. - `<timezone-aware timestamp> AT TIME ZONE zone` returns the wall clock that instant has in `zone`, as a **timezone-naive** `timestamp`. DataFusion lowered both to `CAST(expr AS Timestamp(Nanosecond, Some(tz)))`. That is correct for the first case — arrow's `Timestamp(_, None) -> Timestamp(_, Some(tz))` cast interprets the naive value as local time in `tz`. For the second case the cast between two aware types preserves the instant and only relabels the display zone, so the result stays timezone-aware. The displayed value happens to look right, but the type is wrong, and the moment you do anything with that type the wrongness surfaces: ```sql SET datafusion.execution.time_zone = 'UTC'; CREATE TABLE t AS SELECT '2024-01-01T12:00:00Z'::timestamptz AS tstz; -- before this PR SELECT arrow_typeof(tstz AT TIME ZONE 'America/Denver'), tstz AT TIME ZONE 'America/Denver' FROM t; -- Timestamp(ns, "America/Denver") | 2024-01-01T05:00:00-07:00 -- PostgreSQL: type `timestamp` (naive), value 2024-01-01 05:00:00 SELECT (tstz AT TIME ZONE 'America/Denver')::timestamp FROM t; -- 2024-01-01T12:00:00 <-- the UTC wall clock, not Denver's -- PostgreSQL: 2024-01-01 05:00:00 ``` The `::timestamp` chain in the last query is the bug reported in #12218: arrow's aware→naive cast yields the UTC wall clock, so the timezone the user asked for is silently discarded. All behaviour claimed here was verified against a live PostgreSQL 17.11 (`docker run --rm -e POSTGRES_PASSWORD=pw -p 55432:5432 postgres:17`), for the naive input, the aware input, chaining, fixed offsets, and both DST transitions. ## What changes are included in this PR? `SqlToRel` now types the input of `AT TIME ZONE` (the schema is in scope) and branches on it: - `Timestamp(unit, None)`, or anything that is not a timestamp at all (a string literal, say): unchanged — `CAST(expr AS Timestamp(unit, Some(tz)))`. - `Timestamp(unit, Some(_))`: the same cast, which relabels the instant into `tz` without moving it, wrapped in a step that drops the timezone while keeping the displayed value. That step is exactly `to_local_time`. `datafusion-sql` does not and must not depend on `datafusion-functions`, so the second half goes through a new `ExprPlanner::plan_at_time_zone` hook, implemented by `DatetimeFunctionPlanner` in `datafusion-functions`. ### Design note: `ExprPlanner` hook vs `get_function_meta("to_local_time")` Both routes were available. I picked the `ExprPlanner` hook because: - It is the established precedent for exactly this shape — SQL syntax that lowers to a `datafusion-functions` UDF. `plan_extract` lowers `EXTRACT` to `date_part` the same way, and `datafusion-spark` overrides it to route Spark sessions to a Spark-compatible `date_part`. A future dialect that wants different `AT TIME ZONE` semantics gets the same seam for free. - It keeps the function *name* out of the SQL planner. `get_function_meta("to_local_time")` would bind DataFusion's SQL semantics to whatever UDF an embedder happens to have registered under that name. - Embedders that register a reduced function set get a clear planning error rather than the old silent mislowering: `AT TIME ZONE on a timezone-aware timestamp is not supported by any ExprPlanner. It needs the to_local_time function; register datafusion_functions::datetime (or its DatetimeFunctionPlanner) with the session`. The type branch stays in `datafusion-sql` rather than the hook, because `Expr::get_type` needs the schema and `ExprPlanner` implementations do not have it. The hook therefore receives the already-relabelled expression; that contract is documented on the trait method. The method has a default `PlannerResult::Original` body, so it is not a breaking change for out-of-tree `ExprPlanner` implementations. ### Precision `AT TIME ZONE` no longer hard-codes `Nanosecond`; it keeps the input's `TimeUnit` when the input is a timestamp. `Timestamp(µs, "UTC") AT TIME ZONE 'America/Denver'` now yields `Timestamp(µs)` instead of widening to `Timestamp(ns, "America/Denver")`. Non-timestamp inputs still get `Timestamp(Nanosecond, Some(tz))`, as before. This regressed nothing in the `sqllogictest` corpus. ### Not changed - **Spark.** The planner arm is shared, so a Spark-flavoured session using DataFusion's SQL planner does get the new semantics. But Spark SQL has no `AT TIME ZONE` syntax (it uses `from_utc_timestamp` / `to_utc_timestamp`), and nothing under `datafusion/spark/` implements, tests, or documents it. `SparkFunctionPlanner` is inserted at the *front* of the existing planner list, so `DatetimeFunctionPlanner` is still reached for `plan_at_time_zone`. No Spark-compat behaviour changes. - **`AT LOCAL`.** PostgreSQL 17 supports it (`timestamptz AT LOCAL` is `AT TIME ZONE <session TimeZone>`), but sqlparser 0.62 has no `AtLocal` AST node and rejects the syntax, so there is nothing to route. - **Fixed-offset sign convention.** DataFusion follows arrow/ISO 8601, where `'+05:30'` is east of UTC; PostgreSQL applies the POSIX convention to offsets spelled as strings and reads `'+05:30'` as west. That divergence is pre-existing, orthogonal to this issue, and left alone — only the result *type* changes for these. It is now documented. - **String-literal inputs.** `'2000-12-01T04:04:12-05:00' AT TIME ZONE 'x'` still takes the naive path (`Utf8` is not a timestamp type), which preserves today's behaviour of honouring the offset embedded in the string. PostgreSQL resolves the unknown-typed literal to `timestamp` and drops the offset. Also pre-existing and orthogonal. ## What is the testing strategy for this PR? New `sqllogictest` coverage in `datafusion/sqllogictest/test_files/datetime/timestamps.slt`, added in the first commit as a characterization of today's behaviour and flipped in the second so the diff shows exactly what changed. Every case asserts `arrow_typeof` alongside the value, and every expected value was checked against PostgreSQL 17 by hand — no `--complete` blessing. It covers: a timezone-naive column, a timezone-aware column, the exact `::timestamp` reproducer from #12218, chaining `AT TIME ZONE` twice, a fixed-offset zone, microsecond precision in both directions, `now()`, and a real multi-row timezone-aware column spanning both 2024 DST transitions in `America/Denver` (including the instants either side of the spring-forward gap and the fall-back overlap). Two planner tests in `datafusion/sql/tests/sql_integration.rs`: the naive lowering's plan shape, and the error raised when no `ExprPlanner` provides `to_local_time`. Green locally: the **full** `sqllogictest` suite (505 files) with **zero** pre-existing expectations changed, `cargo test -p datafusion-sql`, `cargo test -p datafusion-functions to_local_time`, `cargo fmt --all`, `cargo clippy --all-targets -- -D warnings`, and the repo's extended workspace suite (`--features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption`). ## Are there any user-facing changes? **Yes — this is a breaking change to `AT TIME ZONE` semantics.** It changes the result *type*, and therefore the result *value* of anything downstream, when the input is already timezone-aware. Timezone-**naive** input — unchanged: ```sql SELECT '2024-01-01 12:00:00'::timestamp AT TIME ZONE 'America/Denver', arrow_typeof('2024-01-01 12:00:00'::timestamp AT TIME ZONE 'America/Denver'); -- before AND after: 2024-01-01T12:00:00-07:00 | Timestamp(ns, "America/Denver") -- PostgreSQL: 2024-01-01 19:00:00+00 | timestamp with time zone ``` Timezone-**aware** input — changed: ```sql SET datafusion.execution.time_zone = 'UTC'; SELECT '2024-01-01T12:00:00Z'::timestamptz AT TIME ZONE 'America/Denver', arrow_typeof('2024-01-01T12:00:00Z'::timestamptz AT TIME ZONE 'America/Denver'); -- before: 2024-01-01T05:00:00-07:00 | Timestamp(ns, "America/Denver") -- after: 2024-01-01T05:00:00 | Timestamp(ns) -- PostgreSQL: 2024-01-01 05:00:00 | timestamp without time zone SELECT ('2024-01-01T12:00:00Z'::timestamptz AT TIME ZONE 'America/Denver')::timestamp; -- before: 2024-01-01T12:00:00 -- after: 2024-01-01T05:00:00 -- PostgreSQL: 2024-01-01 05:00:00 ``` Reference implementations: **PostgreSQL** (verified against 17.11 locally) and **DuckDB**, which agree with each other. Secondary user-facing changes: - `AT TIME ZONE` preserves the input's `TimeUnit` instead of forcing `Nanosecond` (see above). - Sessions that do not register `datafusion-functions`' datetime planner now get a planning error for `AT TIME ZONE` on a timezone-aware input, where before they got a (wrong) plan. - New `AT TIME ZONE` section in `docs/source/user-guide/sql/operators.md`; `to_local_time`'s description clarifies how the two relate. Its examples all apply `AT TIME ZONE` to timezone-*naive* values, so they are unaffected. - New `ExprPlanner::plan_at_time_zone` trait method. It has a default body, so out-of-tree implementations keep compiling. I could not apply the `api change` label myself (no write access to apache/datafusion) — could a committer add it? Flagging it here so it is not missed. **Maintainer input wanted:** should this land as a straight behaviour fix, or behind a config flag (e.g. `datafusion.sql_parser.<something>`) with a deprecation period? I have deliberately *not* added a flag — the current behaviour has no defensible reading, and a flag would mean carrying two timestamp typings through the planner indefinitely. But this will change results for anyone relying on the old shape, so I would rather you decide than assume. 🤖 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]
