adriangb opened a new pull request, #11054: URL: https://github.com/apache/arrow-rs/pull/11054
# Which issue does this PR close? - Closes https://github.com/apache/arrow-rs/issues/11039. > [!IMPORTANT] > **This stacks on #11038 and should be reviewed and merged after it.** It is branched off `fix-dst-timezone-cast`, so the diff here includes #11038's commit; review only the second commit, or wait for #11038 to merge and this will rebase down to one commit. > > Together, #11038 and this PR close https://github.com/apache/datafusion/issues/25084. Neither closes it alone — see below. # Rationale for this change `string_to_datetime` in `arrow-cast/src/parse.rs` resolves a wall clock reading with `LocalResult::single()`, which is `None` in two distinct cases: * the reading is **ambiguous** — the repeated hour when the clocks go back; * the reading is **nonexistent** — the skipped hour when the clocks go forward. There are three such call sites, one for each shape of input: | # | site | example input | | - | ---- | ------------- | | 1 | date-only, reads local midnight | `'2018-11-04'` | | 2 | naive datetime, no trailing offset or zone | `'2024-03-10 02:30:00'` | | 3 | datetime with a trailing IANA zone name | `'2024-03-10 02:30:00 America/New_York'` | Every `Utf8` / `LargeUtf8` / `Utf8View` → `Timestamp(_, Some(tz))` cast goes through one of them. Today such values fail with ``` Error parsing timestamp from '2024-03-10 02:30:00': error computing timezone offset ``` under `CastOptions { safe: false }`, and **silently become NULL** under `safe: true`. #11038 fixed the identical bug in the *cast kernel* (`Timestamp(_, None)` → `Timestamp(_, Some(tz))`), but not this *parser* path. Verified against DataFusion 55.0.0, these are the same query to a user and only the first is fixed by #11038: ```sql SELECT '2024-03-10 02:30:00'::timestamp AT TIME ZONE 'America/New_York'; -- cast kernel: fixed by #11038 SELECT '2024-03-10 02:30:00' AT TIME ZONE 'America/New_York'; -- parser: this PR SET datafusion.execution.time_zone = 'America/New_York'; SELECT '2024-03-10 02:30:00'::timestamptz; -- parser: this PR ``` Landing #11038 on its own leaves an engine where inserting an explicit `::timestamp` mid-expression makes a query start working, which is harder to explain than the current uniform failure. That is why this is a blocker rather than a follow-up. # What changes are included in this PR? **The resolution policy is deliberately shared with #11038, not restated.** Two copies of this policy drifting apart is precisely the bug being avoided here — there is already one such divergence in the tree (`arrow_array::types::from_naive_datetime` picks the *earlier* ambiguous instant and gives up on gaps; left alone in this PR, but worth a follow-up). So #11038's `resolve_local_offset` moves out of `cast/mod.rs` into a new private `arrow-cast/src/local_time.rs` module and becomes `pub(crate)`, generalised from `&Tz` to any `chrono::TimeZone` because `string_to_datetime` is generic over its target timezone. A thin `resolve_local_datetime` wrapper on top of it returns a `DateTime<T>`, which is the shape the three parser sites want. All four call sites — the cast kernel plus the three parser sites — now go through the one function. The policy itself is unchanged from #11038: * **Ambiguous** → the **later** instant, i.e. the offset in effect after the transition (PostgreSQL / DuckDB behaviour). * **Nonexistent** → shifted forward by the length of the gap, recovered by probing the same lookup 24 hours earlier and taking the earliest result. The 24-hour probe is safe: across all 597 `chrono-tz` zones, no two transitions are closer than **167 hours**. (Verified exhaustively while validating #11038; this PR does not re-derive it.) ### On the third site, the zone named in the string Site 3 resolves against a zone the user spelled out in the string, rather than one taken from the target type, so it is fair to ask whether it deserves stricter treatment. It gets the **same** policy, for two reasons: 1. Naming the zone explicitly makes the *zone* unambiguous; it does nothing to make the *wall clock reading* unambiguous. `'2024-03-10 02:30:00 America/New_York'` is exactly as unresolvable as `'2024-03-10 02:30:00'` read in `America/New_York`, and a user who writes it has no other spelling available to express what they meant. 2. PostgreSQL 17 makes no distinction between the two spellings. `SET TimeZone='America/New_York'; SELECT '2024-03-10 02:30:00'::timestamptz;` and `SELECT timestamptz '2024-03-10 02:30:00 America/New_York';` both return `2024-03-10 07:30:00+00`. Treating site 3 differently would reintroduce, inside a single function, the very "same query, different spelling, different outcome" split this PR exists to remove. # Are these changes tested? Yes. **Every expected instant in the new tests was taken from a real PostgreSQL 17.11** (`postgres:17`), not derived by hand. All three call sites are covered for both an ambiguous and a nonexistent reading: * **`America/New_York`** / **`America/Los_Angeles`** — a whole-hour DST step. * **`Australia/Lord_Howe`** — a **thirty minute** step, so a gap shifts `02:15` → `02:45`, not `03:15`. This is what rules out any implementation that assumes a one-hour gap. * **`Pacific/Chatham`** — `+12:45` / `+13:45`, an offset that is not a whole number of hours on either side. * **`Australia/Sydney`** — southern hemisphere, so the gap is in October and the repeated hour in April. This catches implementations that reach for `offset_from_utc_datetime` as a shortcut, which returns the post-transition offset and is wrong here. * **The date-only site specifically**, via the two zones whose transitions land on midnight and are therefore unreachable from any datetime input: * `America/Sao_Paulo` `2018-11-04` — local midnight **does not exist** (Brazil started DST at 00:00) → `2018-11-04T03:00:00Z`. * `America/Havana` `2024-11-03` — local midnight **happens twice** (Cuba ends DST at 00:00) → the later, `2024-11-03T05:00:00Z`. * **A fixed offset** (`+05:30`), which has no transitions and must be unaffected. * **`CastOptions { safe: true }` vs `safe: false`** at the array level: previously the former silently produced NULL and the latter errored; now both produce the resolved instant, while genuinely unparseable input still nulls / errors as before. * **An array straddling both of a zone's 2024 transitions**, across `StringArray`, `LargeStringArray` and `StringViewArray`, confirming the offset is resolved **per row** and not once for the array. * Sub-second precision surviving the shift applied to a nonexistent reading. Counterfactually validated: with the three call sites reverted and the tests kept, 9 of the new tests fail — and the `safe: true` case fails with `left: 0`, the silent-NULL mode. ``` cargo test -p arrow-cast # and --all-features, --doc, --doc --no-default-features cargo clippy -p arrow-cast --all-targets --all-features -- -D warnings cargo test -p arrow --features chrono-tz cargo test -p arrow-csv -p arrow-json -p arrow-array cargo fmt --all ``` `arrow-cast` already gained `arrow-array/chrono-tz` as a dev-dependency in #11038, so no manifest change is needed here. # Are there any user-facing changes? Yes — this is a behaviour change, and an intentional one. Strings that previously failed to parse, or silently became NULL, now parse to the instant PostgreSQL and DuckDB produce. No API changes; `resolve_local_offset` remains crate-private. **One existing test changes expectation.** `arrow/tests/timezone.rs` asserted the old error for two `America/Los_Angeles` cases; they move from `test_parse_timezone_invalid` to `test_parse_timezone`: | input | before | after (= PostgreSQL 17) | | ----- | ------ | ----------------------- | | `2023-03-12 02:05:06 America/Los_Angeles` | `error computing timezone offset` | `2023-03-12T10:05:06+00:00` | | `2023-11-05 01:30:06 America/Los_Angeles` | `error computing timezone offset` | `2023-11-05T09:30:06+00:00` | **Downstream:** DataFusion has one matching expectation, at `datafusion/sqllogictest/test_files/datetime/timestamps.slt:2360-2366`, which asserts that `SELECT TIMESTAMPTZ '2023-03-12 02:00:00 America/Los_Angeles'` errors. Its own comment already notes `# postgresql: accepts`. On the next arrow upgrade it becomes a `query P` returning `2023-03-12T10:00:00Z` — which is what PostgreSQL returns. Flagging it here; no DataFusion change is included in this PR. 🤖 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]
