adriangb commented on PR #25171: URL: https://github.com/apache/datafusion/pull/25171#issuecomment-5626558519
Self-review QA pass on my own PR. I built `datafusion-cli` from this branch and checked every mapping claim against it. Two problems need a fix. Four are smaller. ## 1. The text calls a defect an open question (must fix) The new paragraph ends: > Whether that should remain the mapping is an open question tracked in [issue #25166]. https://github.com/apache/datafusion/issues/25166 carries the `bug` label. Its "Expected behavior" section states an invariant, not a preference: > `TIMESTAMP WITH TIME ZONE` never resolves to `Timestamp(_, None)`. "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 https://github.com/apache/datafusion/pull/18359. This is the whole risk on this PR. I withdrew https://github.com/apache/datafusion/pull/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: | `TIMESTAMPTZ` or `TIMESTAMP WITH TIME ZONE`, optionally with `(p)` | `Timestamp(unit, tz)` | Measured on this branch, default configuration: ``` > select arrow_typeof('2000-01-01T00:00:00'::TIMESTAMPTZ); Timestamp(ns) ``` The cell promises a zone. The default gives none. The prose below corrects it, but a table is the part people scan. The `TIMESTAMP` row directly above says `Timestamp(unit, None)`, so the contrast inside the table reads as "this one is zone-aware". That is exactly backwards for a default install. Fix: put the default in the cell, such as `Timestamp(unit, tz)`, or `Timestamp(unit, None)` by default. ## 3. The precision paragraph appears to cover `TIME`, but it does not The paragraph starts "`unit` is determined by the optional precision `p`" and sits under the whole table. The table includes a `TIME` row. `TIME(p)` is not accepted at all: ``` > select arrow_typeof('00:00:00'::TIME(0)); Error: This feature is not implemented: Unsupported SQL type TIME(0) > select arrow_typeof('00:00:00'::TIME(3)); Error: This feature is not implemented: Unsupported SQL type TIME(3) ``` Scope the paragraph to the two `TIMESTAMP` rows. ## 4. "optionally with `(p)`" hides where `(p)` goes The two spellings put the precision in different places. Both work, and the row does not say so: ``` > select arrow_typeof('2000-01-01T00:00:00'::TIMESTAMPTZ(3)); -- Timestamp(ms) > select arrow_typeof(CAST('2000-01-01T00:00:00' AS TIMESTAMP(6) WITH TIME ZONE)); -- Timestamp(µs) ``` `TIMESTAMPTZ(p)` takes it at the end. `TIMESTAMP(p) WITH TIME ZONE` takes it in the middle. A reference table must show both forms. ## 5. `TIMESTAMP WITHOUT TIME ZONE` is absent The PR adds every spelling of the zone-aware form but not the explicit naive form, which also works: ``` > select arrow_typeof(CAST('2000-01-01' AS TIMESTAMP WITHOUT TIME ZONE)); Timestamp(ns) ``` Add it to the `TIMESTAMP` row. ## 6. The table is where a migrant looks, and the divergence is not there I measured both reference engines rather than quote their docs. PostgreSQL 17.11: ``` => CREATE TABLE t(b TIMESTAMPTZ, c TIMESTAMP WITH TIME ZONE); => SELECT column_name, data_type FROM information_schema.columns WHERE table_name='t'; b | timestamp with time zone c | timestamp with time zone => SELECT pg_typeof('2024-01-01'::timestamptz); timestamp with time zone ``` DuckDB 1.5.2: ``` D SELECT typeof('2024-01-01 00:00:00'::TIMESTAMPTZ); TIMESTAMP WITH TIME ZONE ``` Both engines always resolve the type to a zone-aware type. Neither has an unset session zone. DataFusion resolves it to a zone-naive type on a default install. A user who moves from either engine reads this table first, so the divergence belongs in it, in one clause. The precision rules also diverge, which is worth one line for the same reason: - PostgreSQL accepts `p` from 0 to 6, so `TIMESTAMP(1)` and `TIMESTAMP(4)` are valid. `TIMESTAMP(9)` warns and reduces to 6. - DuckDB accepts 0 to 9 and rounds up to the next storage unit, so `TIMESTAMP(1)` gives `timestamp_ms` and `TIMESTAMP(7)` gives `timestamp_ns`. - DataFusion accepts only 0, 3, 6 and 9, and rejects the rest. ## What I verified, and it is correct Every mapping claim in the diff holds. Measured on a `cargo build --bin datafusion-cli` build of this branch, not read off the source. Default configuration (`datafusion.execution.time_zone` is `NULL`): ``` > select arrow_typeof('2000-01-01T00:00:00'::TIMESTAMP); -- Timestamp(ns) > select arrow_typeof('2000-01-01T00:00:00'::TIMESTAMP(0)); -- Timestamp(s) > select arrow_typeof('2000-01-01T00:00:00'::TIMESTAMP(3)); -- Timestamp(ms) > select arrow_typeof('2000-01-01T00:00:00'::TIMESTAMP(6)); -- Timestamp(µs) > select arrow_typeof('2000-01-01T00:00:00'::TIMESTAMP(9)); -- Timestamp(ns) > select arrow_typeof('2000-01-01T00:00:00'::TIMESTAMP(1)); -- Error: Unsupported SQL type TIMESTAMP(1) > select arrow_typeof('2000-01-01T00:00:00'::TIMESTAMP(2)); -- Error: Unsupported SQL type TIMESTAMP(2) > select arrow_typeof('2000-01-01T00:00:00'::TIMESTAMPTZ); -- Timestamp(ns) > select arrow_typeof(CAST('2000-01-01T00:00:00' AS TIMESTAMP WITH TIME ZONE)); -- Timestamp(ns) > select arrow_typeof('2000-01-01'::DATE); -- Date32 > select arrow_typeof('00:00:00'::TIME); -- Time64(ns) > select arrow_typeof(INTERVAL '1' DAY); -- Interval(MonthDayNano) ``` With the setting present: ``` > SET datafusion.execution.time_zone = 'America/New_York'; > select arrow_typeof('2000-01-01T00:00:00'::TIMESTAMPTZ); -- Timestamp(ns, "America/New_York") > select arrow_typeof(CAST('2000-01-01T00:00:00' AS TIMESTAMP(3) WITH TIME ZONE)); -- Timestamp(ms, "America/New_York") > select arrow_typeof('2000-01-01T00:00:00'::TIMESTAMP); -- Timestamp(ns) > select arrow_typeof('2000-01-01T00:00:00'::TIMESTAMP(0)); -- Timestamp(s) ``` The DDL path agrees: ``` > CREATE TABLE t(a TIMESTAMP, b TIMESTAMPTZ, c TIMESTAMP(6), d TIMESTAMP(0) WITH TIME ZONE); > describe t; a | Timestamp(ns) b | Timestamp(ns) c | Timestamp(µs) d | Timestamp(s) ``` Other checks: - `./ci/scripts/doc_prettier_check.sh` passes. The table rows are aligned at 105 columns each, so the diff only looks ragged. - `../configs.md` is the link style the neighbouring pages already use (`explain.md`, `ddl.md`, `format_options.md`). - The `planner.rs` comment now matches the expression below it. It is a comment only, so no behaviour moves. ## One adjacent nit, pre-existing The untouched `INTERVAL` row says `Interval(IntervalMonthDayNano)`. `arrow_typeof` prints `Interval(MonthDayNano)`. The PR already edits this table, so the row can be corrected in the same commit. ## Merge order No conflict applies. https://github.com/apache/datafusion/pull/25175 adds only `datafusion/sqllogictest/test_files/datetime/timestamps_timezone.slt`, and this PR touches `planner.rs` and `data_types.md`, so the two do not overlap at file level. There is a shared dependency worth a note in the body. Section 0 to 4 of #25175 pins the same naive result that this paragraph describes. A fix for https://github.com/apache/datafusion/issues/25166 must update both places in one change. -- 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]
