fix: choose the AT TIME ZONE branch from the final input type (stacked on apache/datafusion#25165) - #81
Closed
adriangb wants to merge 10 commits into
Closed
fix: choose the AT TIME ZONE branch from the final input type (stacked on apache/datafusion#25165)#81adriangb wants to merge 10 commits into
AT TIME ZONE branch from the final input type (stacked on apache/datafusion#25165)#81adriangb wants to merge 10 commits into
Conversation
Records DataFusion's current behaviour for `<tz-aware> AT TIME ZONE zone`, including the reproducer from apache#12218, DST transitions on a real multi-row column, chaining, fixed offsets and precision handling. PostgreSQL returns a timezone-*naive* `timestamp` here, holding the wall clock in `zone`; DataFusion returns a timezone-aware value that merely relabels the display zone. These expectations are flipped in the next commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`expr AT TIME ZONE 'tz'` was unconditionally lowered to `CAST(expr AS Timestamp(Nanosecond, Some(tz)))`. That is right for a timezone-naive input -- arrow's `Timestamp(_, None) -> Timestamp(_, Some(tz))` cast reads the naive value as local time in `tz` -- but wrong for a timezone-aware one, where it merely relabels the display zone. PostgreSQL (and DuckDB) make the operator asymmetric: `timestamptz AT TIME ZONE zone` returns a `timestamp` (naive) holding the wall clock in `zone`. Because DataFusion kept the value timezone-aware, casting the result to `::timestamp` produced the UTC wall clock instead of the zone's, which is what apache#12218 reports. The SQL planner now types the input and branches on it. The naive case is unchanged. The aware case relabels the instant into `tz` with the same cast and then drops the timezone while keeping the displayed value, which is exactly `to_local_time`. `datafusion-sql` must not depend on `datafusion-functions`, so that second half goes through a new `ExprPlanner::plan_at_time_zone` hook -- the same shape as `plan_extract` lowering `EXTRACT` to `date_part` -- implemented by `DatetimeFunctionPlanner`. Sessions without it get a clear planning error instead of the old silent mislowering. `AT TIME ZONE` also no longer forces `Nanosecond`: it keeps the input's `TimeUnit` when the input is a timestamp. Closes apache#12218 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds an `AT TIME ZONE` entry to the SQL operators guide covering both directions (naive -> aware, aware -> naive), precision preservation, chaining and the ISO-vs-POSIX sign convention for fixed-offset strings. `to_local_time`'s own examples all apply `AT TIME ZONE` to timezone-*naive* values, so they are unaffected by the semantic change; its description now says so, and notes that wrapping an already timezone-aware `AT TIME ZONE` in `to_local_time` is redundant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of this PR found that the fix does not reach every expression. The type
branch calls `Expr::get_type` in the SQL planner, which runs before the type
coercion analyzer. For a `CASE`, `get_type` reports the first non-null `THEN`
arm and ignores coercion.
So these two have the same coerced input type, `Timestamp(ns, "UTC")`, and give
different answers:
CASE WHEN b THEN naive ELSE aware END AT TIME ZONE 'America/Denver'
-> Timestamp(ns, "America/Denver") 2024-01-01T05:00:00-07:00
CASE WHEN b THEN aware ELSE naive END AT TIME ZONE 'America/Denver'
-> Timestamp(ns) 2024-01-01T05:00:00
PostgreSQL gives the naive `2024-01-01 05:00:00` for both. `coalesce` is not
affected, because `verify_function_arguments` coerces before the planner sees
the type.
A correct fix has to dispatch after coercion, which is a larger change than
this PR. Pinning the behaviour so it is visible rather than silent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The characterization suite from apache#25175 pinned the old result, a tz-aware value relabelled to the target zone. It now returns the naive wall clock, as PostgreSQL does, so SECTION 5b changes and loses its divergence note. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Address review: the dispatch reads the type of the input, so pin the result type and value for NULL::timestamp and NULL::timestamptz. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… ZONE` PostgreSQL, DuckDB and Redshift all define `expression AT TIME ZONE zone` as `timezone(zone, expression)`. The function picks its result type from the input type after type coercion, and `simplify` lowers it to the same `CAST` / `to_local_time(CAST)` shape that the SQL planner emits today. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The SQL planner chose between the naive and the aware lowering from `Expr::get_type`, which runs before type coercion. So a `CASE` or `UNION` whose first arm is naive, but whose coerced type is aware, kept the old aware-in, aware-out result. The same early type also set the `TimeUnit`, so a first arm with a coarser unit truncated the other arms. The planner now plans `AT TIME ZONE` as `timezone(zone, expr)` through `ExprPlanner::plan_at_time_zone`, which now receives the raw operands. The function decides once the coerced type is known. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`timezone` lowered itself to `CAST` / `to_local_time(CAST)` in `simplify`. That fixed the branch from the type the simplifier saw, and that type is not always final: - `PREPARE` runs the optimizer without the analyzer, so a `CASE` of a naive and an aware column is still uncoerced there. - An untyped placeholder has no type until `EXECUTE`. Remove the lowering. The branch is now chosen only from the argument type, in `return_field_from_args` and `invoke_with_args`. The SQL planner no longer falls back to reading the input type when no `ExprPlanner` handles `AT TIME ZONE`. It returns an error, as for `EXTRACT`. Without the `CAST`, `unwrap_cast_in_comparison` no longer relabels a literal compared with `naive AT TIME ZONE 'zone'` (apache#25095), so one existing expectation loses a row that never matched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AT TIME ZONE branch after type coercion (stacked on apache/datafusion#25165)AT TIME ZONE branch from the final input type (stacked on apache/datafusion#25165)
adriangb
marked this pull request as ready for review
September 28, 2026 14:51
adriangb
force-pushed
the
fix-at-time-zone-on-tz-aware-timestamps
branch
from
September 29, 2026 19:14
bf445f0 to
fd1c716
Compare
Member
Author
|
Folded into apache#25165. |
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.
Stacked on apache#25165 (base branch
fix-at-time-zone-on-tz-aware-timestamps). This PR is for evaluation. If we like it, the two commits go into the upstream PR.Which issue does this PR close?
AT TIME ZONEon a timezone-aware timestamp returns a naive timestamp apache/datafusion#25165:AT TIME ZONEchooses its branch before type coercion.Rationale for this change
Terms: an aware timestamp has a timezone (
Timestamp(unit, Some(tz))). A naive timestamp has none (Timestamp(unit, None)).The upstream PR reads the input type in the SQL planner, which runs before type coercion. That type is wrong for a
CASEor aUNIONwhose first arm is naive while the coerced type is aware. It also sets theTimeUnitfrom that first arm.Measured with
datafusion-clibuilds ofmain(33574a1), the upstream PR (bf445f0) and this PR:main(CASE WHEN b THEN aware ELSE naive END) AT TIME ZONE 'America/Denver'05:00:00-07:00aware05:00:00naive05:00:00naive05:00:00naive(CASE WHEN b THEN naive ELSE aware END) AT TIME ZONE 'America/Denver'05:00:00-07:00aware05:00:00-07:00aware ❌05:00:00naive05:00:00naivex AT TIME ZONE 'America/Denver'overSELECT naive AS x UNION ALL SELECT awareCASE WHEN b THEN naive ELSE aware END(CASE WHEN f THEN naive_s ELSE aware_frac END) AT TIME ZONE 'UTC'12:00:00.123456789Z12:00:00Z❌ (fraction lost)12:00:00.123456789The last row is a regression of the upstream PR against
main: the planner casts to the unit of the first arm (seconds), so the other arm loses its fraction of a second.A mix of naive and aware always coerces to aware, so the early type can only miss an aware input. It never reports a naive input as aware.
What changes are included in this PR?
Root cause
The result type of
AT TIME ZONEdepends on the input type. Every failure so far came from one mistake: code chose the branch from a type that was not yet final.CASE,UNION, CTE, view;TimeUnitof the first armsimplify(first version of this PR)PREPAREruns the optimizer without the analyzer; an untyped placeholder has no type untilEXECUTEPREPAREof aCASE;$1 AT TIME ZONE ...The fix removes every early choice.
AT TIME ZONEbecomes the functiontimezone(zone, expr), and that function chooses the branch only from its argument type, at the two places where DataFusion reads that type:return_field_from_args, each time the plan is typed. The physical planner types the final, coerced plan.invoke_with_args, from the data type of the actual input.Nothing rewrites the call into a
CASTduring planning, so no stale choice can stay in the plan.Commits
feat: add timezone(zone, expression). A new scalar function, the function form ofAT TIME ZONE. A dictionary-encoded timestamp follows its value type (item 4 of the upstream self-review). The column is named with the operator syntax:t.x AT TIME ZONE 'America/Denver'.fix: choose the AT TIME ZONE branch after type coercion. The SQL planner no longer reads the input type. It buildstimezone(zone, expr)throughExprPlanner::plan_at_time_zone, which now receives the raw operands[zone, expr], likeplan_extract. That resolves item 3 of the upstream self-review.fix: never fix the AT TIME ZONE branch before the input type is final. Removes thesimplifylowering that the second commit still had, and the SQL planner's no-planner fallback that read the input type. Without anyExprPlannerforAT TIME ZONE, planning now fails for every input, as forEXTRACT.Why the name
timezone(zone, expression)timezone(zone, timestamp), documented as the same asAT TIME ZONEtimezone(text, timestamp),AT TIME ZONEis "syntactic sugar" for itTIMEZONE('zone', timestamp)at_timezone(ts, zone)toTimeZone(ts, zone)CONVERT_TIMEZONE(target, ts)convert_timezone,from_utc_timestamp,CONVERT_TZOnly
timezonehas the same semantics and argument order in the engines thatAT TIME ZONEalready follows. The other names mean something different, so they are not good aliases.Checked shapes
With the final build, each shape below gives the PostgreSQL / DuckDB result for an aware input:
CASEin both arm orders,UNION ALL, a CTE, a view, a view whose body hasAT TIME ZONE, a scalar subquery,GROUP BY/ORDER BY, a filter, a join key, a nestedAT TIME ZONE,max(...),first_value(...) OVER (),coalesce, andPREPARE/EXECUTEwith aCASE, an untyped placeholder, a placeholder inside aCASE, and a typed placeholder.Are these changes tested?
timestamps.sltcases:CASEin both arm orders,UNION ALL, a CTE, a view, an aggregate, mixed time units inCASEandUNION, a dictionary input in both directions,PREPAREwith an untyped placeholder (bound to an aware and to a naive value) and with aCASE,EXPLAIN, the function form, and the error for a non-constant zone. DuckDB 1.5.2 gives the same values for theCASEandUNIONcases.invoke_with_args.sql_integration.rs: the plan withDatetimeFunctionPlanner, and the error without it.cargo test -p datafusion-sql -p datafusion-functions -p datafusion-expr,cargo clippy --all-targets --workspace --features avro,integration-tests,extended_tests -- -D warnings,cargo fmt,dev/update_function_docs.sh, and the doc prettier check.Are there any user-facing changes?
In addition to the upstream PR:
timezone(zone, expression).AT TIME ZONEuses the coerced input type, for both the branch and theTimeUnit.AT TIME ZONEcolumn is namedx AT TIME ZONE 'zone'for every input. Onmaina naive input givesx. The upstream PR givesto_local_time(x)for an aware input.ExprPlanner::plan_at_time_zonereceives[zone, expr]instead of the relabelledCAST. The hook is new in the upstream PR, so no released API changes.ExprPlannerforAT TIME ZONE(only whendatafusion-sqlis used withoutdatafusion-functions), planning fails for every input. Onmainit gave aCAST.timezone(...)wheremainshowedCAST(... AS Timestamp(..., Some(zone))). So the optimizer no longer sees aCASTfornaive AT TIME ZONE 'zone':unwrap_cast_in_comparisonno longer relabels a literal compared with it. That relabel is wrong for this cast (unwrap_cast_in_comparison drops the timezone shift when unwrapping CAST(timestamp AS timestamptz) = literal apache/datafusion#25095). Onetimestamps.sltquery returned a row whose instant is one hour away from the literal; it now returns no row, as in DuckDB.naive AT TIME ZONE 'UTC'or a fixed offset no longer reuses the input ordering. For a named zone, theCASTnever preserved ordering (daylight saving time). If this matters,timezonecan implementoutput_orderingfor fixed-offset zones.🤖 Generated with Claude Code