adriangb commented on PR #25165:
URL: https://github.com/apache/datafusion/pull/25165#issuecomment-5626652492
Self-review: a QA pass on my own PR. I ran every check below against three
sources:
- this branch, built as `datafusion-cli`,
- a live PostgreSQL 17.11 in Docker,
- a local DuckDB 1.5.2 CLI. One finding is a real gap in the fix. The rest
are smaller, plus two claims in the description that I must correct.
Terms: an **aware timestamp** carries a timezone (`Timestamp(unit,
Some(tz))`). A **naive timestamp** carries none (`Timestamp(unit, None)`).
---
## 1. The type branch runs before type coercion, so the fix misses some
inputs
`sql_at_time_zone_to_expr` calls `Expr::get_type` in the SQL planner. That
is before the type-coercion analyzer runs. For a `CASE` expression,
`Expr::get_type` returns the type of the first non-null `THEN` arm and ignores
coercion. So the branch reads "naive" for an expression whose real type is
aware.
```sql
SET datafusion.execution.time_zone = 'UTC';
CREATE TABLE t AS SELECT '2024-01-01T12:00:00Z'::timestamptz AS aware,
'2024-01-01 12:00:00'::timestamp AS naive, true
AS b;
SELECT arrow_typeof(CASE WHEN b THEN naive ELSE aware END) FROM t;
-- Timestamp(ns, "UTC") <- the real type is aware
SELECT arrow_typeof((CASE WHEN b THEN naive ELSE aware END) AT TIME ZONE
'America/Denver'),
(CASE WHEN b THEN naive ELSE aware END) AT TIME ZONE 'America/Denver'
FROM t;
-- Timestamp(ns, "America/Denver") 2024-01-01T05:00:00-07:00 <- the OLD,
wrong shape
SELECT arrow_typeof((CASE WHEN b THEN aware ELSE naive END) AT TIME ZONE
'America/Denver') FROM t;
-- Timestamp(ns) <- correct, only because the arms swap order
```
PostgreSQL 17.11 on the same shape:
```
case_type | v | t
timestamp with time zone | 2024-01-01 05:00:00 | timestamp without time zone
```
So the result depends on the order of the `CASE` arms. `coalesce` is safe,
because `verify_function_arguments` coerces the argument fields before it
computes the return field. `CASE` is not.
Options, in my order of preference:
1. Do the dispatch after type coercion. A rewrite pass sees the coerced
type, so the branch is always right.
2. Keep the planner branch and add the `CASE` case to the `.slt` file as a
known limitation, with a `TODO` and an issue link.
I do not want to merge this without at least option 2. A silent,
order-dependent hole is worse than the old uniform bug.
## 2. The description says PostgreSQL and DuckDB agree on every case. They
do not
The claim covers "fixed offsets". DuckDB rejects a fixed-offset string
outright:
```
D SELECT (TIMESTAMPTZ '2024-01-01 12:00:00Z' AT TIME ZONE '+05:30');
Not implemented Error: Unknown TimeZone '+05:30'!
```
PostgreSQL accepts it and reads it POSIX-style (west-positive), so it
returns `2024-01-01 06:30:00`. DataFusion reads it ISO-style (east-positive)
and returns `2024-01-01 17:30:00`. PostgreSQL agrees with DataFusion only when
the offset is spelled as an `interval`:
```
postgres=# SELECT timestamptz '2024-01-01 12:00:00Z' AT TIME ZONE interval
'+05:30';
2024-01-01 17:30:00
```
The two engines cannot arbitrate this one. The description must say so. It
must not claim agreement. The divergence already exists on main and is out of
scope here (https://github.com/apache/datafusion/issues/25170), but it must not
hide behind a blanket claim.
## 3. The `ExprPlanner` hook is narrower than the description says
The description says a future dialect "gets the same seam for free". It does
not.
- The hook runs only for an aware input. A dialect cannot change the naive
half or the type dispatch.
- The hook receives one `Expr` that is already `CAST(input AS
Timestamp(unit, Some(tz)))`. An implementation that wants other semantics must
destructure that `Cast` to recover the input and the target timezone.
Every sibling hook takes the raw operands. `plan_extract` gets `[field,
expr]`. I suggest `plan_at_time_zone(vec![input, lit(tz)])` and let the
implementation build its own cast. The trait doc then states a contract instead
of a workaround.
Two smaller points on the same code:
- The name is wrong. The method plans the second half only.
`plan_to_local_time` says what it does.
- Use `not_impl_err!` for the "no planner" error, to match the `EXTRACT` arm
at `datafusion/sql/src/expr/mod.rs:321`.
The documented contract itself does hold: the single call site always passes
the relabelled `CAST`.
## 4. A dictionary-encoded aware timestamp keeps the old behaviour
`_ => (TimeUnit::Nanosecond, false)` treats `Dictionary(_, Timestamp(_,
Some(tz)))` as naive:
```sql
CREATE TABLE d AS SELECT
arrow_cast(arrow_cast('2024-01-01T12:00:00Z','Timestamp(Nanosecond,
Some("UTC"))'),
'Dictionary(Int32, Timestamp(Nanosecond,
Some("UTC")))') AS dts;
SELECT arrow_typeof(dts AT TIME ZONE 'America/Denver'), dts AT TIME ZONE
'America/Denver' FROM d;
-- Timestamp(ns, "America/Denver") 2024-01-01T05:00:00-07:00 <- old shape
```
Add a `Dictionary(_, Timestamp(unit, tz))` arm, or state the limit in the
operator doc.
## 5. The output column name changes, and the new name drops the timezone
```
SELECT aware AT TIME ZONE 'America/Denver' FROM t; -- column:
to_local_time(t.aware)
SELECT naive AT TIME ZONE 'America/Denver' FROM t; -- column: t.naive
```
An unaliased projection changes name for the aware case. The new name never
names `America/Denver`, so it misleads the reader. PostgreSQL names the column
`timezone`. This belongs in the user-facing list.
---
## Claims I verified as correct
**Zero changed expectations, and the double edge.** I grepped the whole
tree. `AT TIME ZONE` appears in exactly one test file,
`datafusion/sqllogictest/test_files/datetime/timestamps.slt`. Every use on main
has a `Utf8` or naive inner expression. The two `date_bin(interval '1 day',
to_local_time(column1)) AT TIME ZONE ...` cases qualify because `to_local_time`
returns `Timestamp(unit, None)`. The `sqllogictest` diff has zero deleted lines
across 507 `.slt` files. (The description says 505.)
The double edge is the part the description must own: the corpus had no
coverage of the broken case at all. That is why the bug survived from #9647 in
DataFusion 37. "Zero changed expectations" is weak evidence of safety here, not
strong. https://github.com/apache/datafusion/pull/25175 adds the coverage that
main lacks, and whichever PR merges second must update the other.
**The `to_local_time` doc idiom.** `'2024-04-01T00:00:20Z'::timestamp` types
as `Timestamp(ns, None)`, so the examples take the unchanged branch. Correct.
But the added sentence is backwards. In that idiom `to_local_time` is a
round trip: `to_local_time(<naive> AT TIME ZONE 'X')` returns the same wall
clock as `<naive>`. The doc's own first two examples both print
`2024-04-01T00:00:20`, which shows it. The new text calls the *other* form
redundant and then sends the reader to the round-trip form. Say instead why
`to_local_time` still earns its place: it strips the timezone of a column whose
timezone you cannot name in SQL, and `AT TIME ZONE` needs a literal target.
**The `TimeUnit` change.** Safe, and I checked each step:
- `Coercion::new_exact(TypeSignatureClass::Timestamp)` keeps the unit and
the timezone. `default_casted_type` returns `origin_type` for a timestamp.
- `to_local_time` handles all four units, for both the scalar and the array
path.
- Measured: `Timestamp(µs, "UTC")` gives `Timestamp(µs)`, `Timestamp(s,
"UTC")` gives `Timestamp(s)`, and both values are `2024-01-01T05:00:00`.
- PostgreSQL also keeps the precision of the input, so this closes a real
gap.
It is in scope in spirit, but it is a second type change in a PR whose title
names one. Give it its own bullet in the user-facing list.
**Spark.** No Spark behaviour changes. `SparkFunctionPlanner` implements
`plan_extract` and `plan_substring` only, so `plan_at_time_zone` falls through
to `DatetimeFunctionPlanner`. `with_spark_features` inserts the Spark planner
at index 0 of the list that `with_default_features` builds, and the doc on that
trait requires that order. The description's own wording is in tension though:
it says a Spark session does get the new semantics, then says no Spark-compat
behaviour changes. Say "no Spark-specific function or planner behaviour
changes".
**Other checks that came back clean:**
- `AT LOCAL`: sqlparser 0.62.0 has no `AtLocal` node. Nothing to route.
- No unparser or proto path emits `AT TIME ZONE`, so there is no round-trip
risk.
- An untyped placeholder types as `DataType::Null` and takes the naive
branch. No new error.
- `now()` is naive by default and aware only when
`datafusion.execution.time_zone` is set. The `.slt` comment is right, and the
`SET` is necessary for that case.
- `DatetimeFunctionPlanner` builds the UDF value directly, so an
unregistered `to_local_time` name does not break it.
- A bad timezone still errors clearly: `Invalid timezone "Not/AZone"`.
- All six DST instants match PostgreSQL 17.11 and DuckDB 1.5.2 exactly, on
the value.
## One old bug I hit while I measured
A naive input that lands in a DST gap or a DST overlap errors:
```sql
SELECT '2024-03-10 02:30:00'::timestamp AT TIME ZONE 'America/Denver';
-- Arrow error: Cast error: Cannot cast timezone to different timezone
SELECT '2024-11-03 01:30:00'::timestamp AT TIME ZONE 'America/Denver';
-- same error
```
PostgreSQL returns `2024-03-10 09:30:00+00` and `2024-11-03 08:30:00+00`.
DuckDB returns the same two values. `arrow_cast` alone reproduces the error, so
it comes from the arrow cast and this PR does not change it. It is out of
scope, but it deserves its own issue.
🤖 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]
