docs: document the TIMESTAMP WITH TIME ZONE type mapping and fix a stale comment - #25171
docs: document the TIMESTAMP WITH TIME ZONE type mapping and fix a stale comment#25171adriangb wants to merge 2 commits into
TIMESTAMP WITH TIME ZONE type mapping and fix a stale comment#25171Conversation
The comment on the `SQLDataType::Timestamp` arm in `planner.rs` promises `Timestamp<TimeUnit, Some(Time Zone)>`, but the expression it describes is an `Option<String>` that is `None` by default. That comment went stale in apache#18359, which changed `datafusion.execution.time_zone` from `String` to `Option<String>` and dropped the `Some(...)` wrapper without touching the two lines above it. Correct the comment; the behavior itself is under discussion in apache#25166 and is left unchanged. `docs/source/user-guide/sql/data_types.md` had no row for `TIMESTAMP WITH TIME ZONE` / `TIMESTAMPTZ` at all. Add one, along with the optional `(p)` precision that both it and `TIMESTAMP` accept, and note that the timezone component comes from `datafusion.execution.time_zone`, which is unset by default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Self-review QA pass on my own PR. I built 1. The text calls a defect an open question (must fix)The new paragraph ends:
#25166 carries the
"An open question" reads as a design debate with two defensible sides. The issue does not claim that. It claims a defect with a traceable cause in #18359. This is the whole risk on this PR. I withdrew #25162 for one reason: reference text turns a defect into semantics that a reader must learn. This paragraph repeats a softer form of the same error. A reader comes away with "naive by default is the rule I must learn", not "this mapping is disputed". Fix: name it a defect. For example, "The current mapping is a known defect, tracked in [issue #25166]." One word carries the whole signal. 2. The table row alone teaches the opposite of the default (must fix)The new row says: | Measured on this branch, default configuration: The cell promises a zone. The default gives none. The prose below corrects it, but a table is the part people scan. The Fix: put the default in the cell, such as 3. The precision paragraph appears to cover
|
…later Review feedback on this PR: the table cell said `Timestamp(unit, tz)` while the default configuration produces `Timestamp(unit, None)`, and the row directly above it said `None`. So the table on its own taught the opposite of what happens, and the correction sat two paragraphs below. People scan tables. Also softens nothing: issue 25166 carries the `bug` label and states an invariant, so "an open question" was the wrong register. It is a known bug, and the text now says so and names what PostgreSQL and DuckDB do instead. Adds `TIMESTAMP WITHOUT TIME ZONE`, which works but was absent, and states where the precision goes, which differs between the two spellings. Moves the precision paragraph so it no longer appears to cover `DATE`, `TIME` and `INTERVAL`, none of which accept one. Every mapping re-verified against a `datafusion-cli` build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25171 +/- ##
==========================================
+ Coverage 81.60% 81.91% +0.31%
==========================================
Files 1123 1132 +9
Lines 408898 421117 +12219
Branches 408898 421117 +12219
==========================================
+ Hits 333670 344951 +11281
- Misses 55625 55775 +150
- Partials 19603 20391 +788 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Which issue does this PR close?
This PR closes no issue. It changes documentation and one comment, and it changes no behaviour.
Rationale for this change
What a user hits
There is no bug to reproduce. There is a mapping a user cannot look up. To find out what
TIMESTAMP WITH TIME ZONEproduces, a user must run it:The type has no timezone. Set the session timezone and the same expression gains one:
docs/source/user-guide/sql/data_types.mdholds the SQL-to-Arrow type table. Three things were absent from it:TIMESTAMPTZorTIMESTAMP WITH TIME ZONEdatafusion.execution.time_zoneTIMESTAMP(p)is accepted, and which values ofpare validA user who reads the page learns none of the above.
The comment in
datafusion/sql/src/planner.rswas worse than absent. It stated the opposite of the code:The expression is an
Option<String>that isNoneby default, so the arm producesTimestamp(unit, None)on a default install.What changes for a user
The type table gains the two absent spellings and the precision rules, so a user can look the mapping up instead of a run of
arrow_typeof. The page also states that the timezone component comes from a session setting that is unset by default, and it links the open issue on that default.No behaviour changes.
The technical detail
The comment went stale in #18359, which changed
datafusion.execution.time_zonefromStringtoOption<String>with aNonedefault. That PR dropped theSome(...)wrapper from the expression as a mechanical consequence of the type change, and left the two comment lines above it untouched.Whether the timezone-naive default is right is under discussion in #25166. This PR only makes the documentation match the current code. It deliberately changes no behaviour.
Field research
The table is exactly where a user who moves from another engine looks, so I measured both reference engines.
PostgreSQL 17.11 (
postgres:17):DuckDB 1.5.2:
Both engines always resolve the type to a zone-aware type. Neither has an unset session timezone, so neither can produce a naive result from this SQL type. DataFusion produces a naive result on a default install. That divergence is the subject of #25166.
The precision rules diverge too:
pfrom 0 to 6.TIMESTAMP(1)andTIMESTAMP(4)are valid.TIMESTAMP(9)raises a warning and reducespto 6.pfrom 0 to 9 and rounds up to the next storage unit.TIMESTAMP(1)givestimestamp_ms,TIMESTAMP(4)givestimestampandTIMESTAMP(7)givestimestamp_ns.What changes are included in this PR?
datafusion/sql/src/planner.rs: correct theSQLDataType::Timestampcomment to sayTimestamp<TimeUnit, Time Zone>, note that the configured time zone is anOptionthat is unset by default, and point at TIMESTAMP WITH TIME ZONE can resolve to a timezone-naive type, and casting to it discards an existing timezone #25166. There is no code change.docs/source/user-guide/sql/data_types.md: add theTIMESTAMP WITH TIME ZONEandTIMESTAMPTZrow to the Date/Time Types table, document the optional(p)precision and the units it selects, and state that the timezone component comes fromdatafusion.execution.time_zone, which is unset by default, with a link to the open issue.What is the testing strategy for this PR?
There are no tests. This is a comment fix plus a documentation fix, with no behaviour change.
Every mapping in the docs was verified against a
cargo build --bin datafusion-clibuild of this branch rather than read off the source:The DDL path gives the same answers:
./ci/scripts/doc_prettier_check.sh,cargo fmt --allandcargo clippy --all-targets -- -D warningsall pass. The Sphinx docs build cleanly. Its one warning is the pre-existing absent generated_static/data/deps.svg, which is unrelated to this change.Are there any user-facing changes?
Documentation only. There are no behaviour changes and no API changes.
Merge order
There is no file-level conflict with any PR in flight. #25175 adds only
datafusion/sqllogictest/test_files/datetime/timestamps_timezone.slt, and this PR touchesplanner.rsanddata_types.md.The two do share a dependency. Sections 0 to 4 of #25175 pin the same timezone-naive result that the new paragraph describes. A fix for #25166 must update this page and that file in one change.
🤖 Generated with Claude Code