adriangb opened a new pull request, #25211:
URL: https://github.com/apache/datafusion/pull/25211
## Which issue does this PR close?
- Closes issue 13212.
## Rationale for this change
A `Timestamp(unit, Some(tz))` value is an instant; `tz` is only a display
label
attached to it. A `Timestamp(unit, None)` value is a wall clock reading with
no
instant attached. Converting the second into the first has to pick a zone to
read the wall clock in, and that choice decides which instant you get.
PostgreSQL and DuckDB both read it in the **session** time zone at every
implicit conversion. DataFusion reads it in whatever zone happens to label
the
*other operand*, because type coercion emits a plain
`CAST(naive AS Timestamp(u, Some(tz)))` and arrow's cast reads the naive
value
in `tz`. So with `datafusion.execution.time_zone` set, DataFusion answers a
query differently from both of them:
```sql
SET TIME ZONE = '+08';
-- PostgreSQL 17 and DuckDB 1.5.2: 08:00:00
-- DataFusion before this PR: 00:00:00
SELECT '2024-11-01T00:00:00+00:00'::timestamptz -
'2024-11-01T00:00:00'::timestamp;
```
The same disagreement shows up wherever a naive timestamp meets an aware one:
comparisons, `-`, `IN`, `BETWEEN`, `CASE`, `UNION`, `coalesce`,
`greatest`/`least`, `nullif`, the array functions, `date_bin`'s origin,
`VALUES`
and `INSERT`.
## What changes are included in this PR?
Wherever the planner or the analyzer **inserts** a cast from a (possibly
nested)
naive timestamp to an aware type whose zone differs from
`datafusion.execution.time_zone`, a two step cast is emitted instead of one:
```text
CAST(ts AS Timestamp(ns, "America/New_York"))
-- becomes, with datafusion.execution.time_zone = '+08:00'
CAST(CAST(ts AS Timestamp(ns, "+08:00")) AS Timestamp(ns,
"America/New_York"))
```
The inner cast reads the wall clock in the session zone; the outer one only
relabels, since an aware -> aware cast preserves the instant. The coerced
*type* is therefore unchanged and only the instant differs. Nested naive
timestamps (`List`, `LargeList`, `FixedSizeList`, `Struct`, `Map`,
`Dictionary`)
are re-zoned leaf by leaf.
**Explicit** conversions keep arrow's semantics untouched, which is also what
PostgreSQL and DuckDB do for their explicit forms: `AT TIME ZONE`,
`arrow_cast`,
the DataFrame API's `cast_to`/`cast`, and Substrait casts. SQL
`CAST(x AS TIMESTAMPTZ)` already targeted the session time zone.
The distinction is exact by construction, not by inspection: the two step
form
is built at the point the cast is inserted, never by looking for inserted
casts
afterwards. A cast is only ever split by the code that created it, and an
expression that already has the target type is never cast at all, so a cast
the
user wrote can never be mistaken for one DataFusion inserted. The three
library
functions that insert a cast into a whole plan rather than into one
expression
each gained a session-time-zone aware sibling for that reason.
The commits are:
1. `Timestamp(u, Some(tz)) - Timestamp(u, None)` at *equal* units is the one
mixed pair arrow can subtract directly, so `BinaryTypeCoercer` inserted no
cast at all and the naive operand was read as UTC — disagreeing with `=`
on
the same values and with every other unit pairing. It now coerces like the
rest.
2. The `cast_to_with_session_time_zone` helper in `datafusion-expr`, plus a
sibling for each of the three library functions that build a whole plan of
casts rather than one cast: `coerce_plan_expr_for_schema`, `cast_subquery`
and `LogicalPlanBuilder::values`/`values_with_schema`. Each sibling takes
the
session time zone and calls the helper at the exact point it inserts a
cast;
the existing functions become thin wrappers passing `None`, so their
signatures and behaviour are unchanged.
3. Every cast the analyzer inserts routed through it, opted into with the new
`TypeCoercionRewriter::with_session_time_zone`. `ExprSimplifier::coerce`
passes the session zone too, so `SessionContext::create_physical_expr`
behaves like SQL. The two optimizer rules that build a rewriter over an
already-coerced plan deliberately do not. A scalar function argument that
is
already a literal was folded to the coerced type outright rather than
wrapped
in a cast, which read its wall clock in the coerced type's own zone; it
now
keeps the split cast when there is one, and is folded as before when
there is
not.
4. The SQL planner's own insertion points: `INSERT`, `UPDATE ... SET` and
`VALUES`. `LogicalPlanBuilder`'s existing API is unchanged.
5. Tests.
6. Docs.
`datafusion.execution.time_zone` is unset by default, and with no session
time
zone every plan is byte-identical to today's.
## What is the testing strategy for this PR?
- Unit tests for the helper in
`datafusion/expr/src/type_coercion/session_time_zone.rs`: each nested type
and
the explicit-cast and no-session-zone no-ops.
- Snapshot tests in `datafusion/optimizer/src/analyzer/type_coercion.rs`
pinning
the analyzed plan for subtraction, `= ANY (<subquery>)`, `UNION`, `CASE`
and a
coerced function argument — each paired with the plan produced with no
session
time zone, which must stay exactly what DataFusion produced before. A
`UNION`
branch and a subquery projection that are *already* an explicit cast to the
common type are pinned too, and must not be split.
- Plan snapshots in `datafusion/sql/tests/sql_integration.rs` for a `VALUES`
list under a session time zone: the naive cell carries the two step cast
and
the `AT TIME ZONE` cell is untouched.
- A section in `datafusion/sqllogictest/test_files/datetime/timestamps.slt`
covering every site end to end, with each expectation being the instant
PostgreSQL and DuckDB return. It also pins the plans, so that the two
casts are
visible and provably survive `simplify_expressions`; the default behaviour
with
no session time zone; a named session zone across a DST boundary (EST and
EDT); and `AT TIME ZONE` / `arrow_cast`, which must be unaffected —
including
an `AT TIME ZONE` at the top of a `UNION` branch, a subquery projection, a
`VALUES` cell and a view.
- `test_create_physical_expr_timestamp_subtraction_uses_session_timezone` in
`datafusion/core/tests/expr_api/mod.rs` for the `create_physical_expr`
path.
- The full sqllogictest suite (516 files), `datafusion-substrait` and
`datafusion-proto` all pass, the latter two confirming that plans carrying
the
nested casts round-trip.
One pre-existing expectation was wrong and is corrected:
`date_bin('1 day', TIMESTAMPTZ '2022-01-01 20:10:00Z', TIMESTAMP
'2020-01-01')`
under `+07` was pinned at `2022-01-01T07:00:00+07:00`, i.e. the naive origin
read
as UTC. PostgreSQL and DuckDB both return `2022-01-02T00:00:00+07:00`, which
is
also what the TIMESTAMPTZ-origin case immediately above it already expected.
## Are there any user-facing changes?
Yes, and they are documented in the 56.0.0 upgrade guide.
- With `datafusion.execution.time_zone` set, a timezone-naive timestamp is
read
in that zone wherever DataFusion implicitly converts it to a timezone-aware
one. The default is unset, which keeps the previous behaviour.
- `Timestamp(u, Some(tz)) - Timestamp(u, None)` at equal units now reads the
naive operand in the aware operand's zone even with no session time zone,
rather than as UTC.
- Under a **named** session time zone, a naive wall clock value that falls
in a
daylight saving gap or fold currently errors with arrow's
`Cannot cast timezone to different timezone` instead of resolving to an
instant. This is a pre-existing arrow limitation that a named session time
zone
now reaches in more places; it is fixed by
<https://github.com/apache/arrow-rs/pull/11038> and
<https://github.com/apache/arrow-rs/pull/11054>, tracked on the DataFusion
side
at <https://github.com/apache/datafusion/issues/25084>. Fixed offsets are
unaffected.
New public API (no breaking changes):
-
`datafusion_expr::type_coercion::session_time_zone::cast_to_with_session_time_zone`
-
`datafusion_expr::expr_rewriter::coerce_plan_expr_for_schema_with_session_time_zone`
- `datafusion_expr::expr_schema::cast_subquery_with_session_time_zone`
- `datafusion_expr::LogicalPlanBuilder::values_with_session_time_zone`
- `TypeCoercionRewriter::with_session_time_zone`. Applying the session time
zone
is opt in: `TypeCoercionRewriter::new` alone coerces exactly as it did
before,
so callers who coerce expressions themselves and want to match SQL planning
need to call it.
🤖 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]
