adriangb commented on PR #11054:
URL: https://github.com/apache/arrow-rs/pull/11054#issuecomment-5626606676
Self-review QA pass — this is our own PR, so treat this as our own
adversarial check, not an independent review.
No blocking defect. The highest-risk part of this stack is the change of
`resolve_local_offset` from `&Tz` to `<T: TimeZone>`, so I attacked that first.
It is behaviour-preserving, and I verified that empirically rather than by
reading. Four findings follow, in priority order.
## 1. The generalisation does not change the original cast call site —
verified byte for byte
This was the item that could regress #11038 silently. I ran the identical
battery of cast-path readings on both branches and compared the raw output:
* 15 readings in one array that straddles both 2024 `America/New_York`
transitions.
* 15 readings across `Australia/Sydney`, `Australia/Lord_Howe`,
`Pacific/Chatham`, `America/Sao_Paulo`, `America/Havana`, `Europe/Lisbon`,
`Europe/Dublin`, `Asia/Tehran`, `Pacific/Apia`, `America/Santiago` and
`Asia/Kolkata`.
* 13 readings inside the four 24-hour dateline gaps.
43 rows, and the two files are identical. The cast path on this branch is
bit-for-bit the cast path on #11038.
The refactor moves `.fix()` from inside the function to the call site.
`Option::map(|o| o.fix())` and then `?`, versus `?` and then `.fix()`, are the
same operation in the same order. The `Single`, `Ambiguous` and `None` arms are
otherwise unchanged. So the empirical result matches the reading of the diff.
One note on the new wrapper. `resolve_local_datetime` builds its result with
`tz.from_utc_datetime(&(*local - offset.fix()))`, not with
`from_local_datetime`. That is deliberate and necessary — `from_local_datetime`
would reject the two cases this function exists to resolve — but it is not
obvious. A one-line comment at that call helps the next reader.
## 2. The stated safety argument for the 24-hour probe is not the
load-bearing one
This PR repeats #11038's justification: no two transitions are closer than
167 hours. That figure is correct, and I reproduce it. But it is not the
property that makes the probe resolvable.
Two separate invariants are at work:
* **Transition spacing above 24 hours.** This makes the probed offset the
*correct* pre-transition offset. Margin: 167 hours.
* **Maximum local gap at or below 24 hours.** This makes the probe land
*outside* the gap at all. This PR does not state it, and it has no margin.
Seven zones have a local gap of exactly 24.00 hours — the dateline changes:
`Pacific/Apia`, `Pacific/Fakaofo`, `Pacific/Kiritimati`, `Pacific/Kanton`,
`Pacific/Enderbury`, `Pacific/Kwajalein` and `Kwajalein`. The next largest gap
in any zone is 10.00 hours. For a reading at the last second of a 24-hour gap
the probe lands one second before it. The real margin is one second.
The code is correct today. I tested the first instant, the midpoint and the
last second of all four surviving dateline gaps through the cast path, and all
13 readings match PostgreSQL 17.11 exactly. But the doc comment in
`local_time.rs` now carries this claim for the whole crate, so please state
both invariants there.
## 3. The give-up branch is unreachable, and this PR widens who depends on
that
`resolve_local_offset` returns `None` only when the probe itself lands in a
gap. Both invariants above exclude that, so the branch is dead code with real
tzdata. I could not construct an input that reaches it.
This PR raises the stakes, because the branch now has four callers rather
than one. I read all four. All handle `None` correctly:
| Caller | `None` behaviour |
| --- | --- |
| `adjust_timestamp_to_timezone`, `safe: false` | `CastError("Cannot cast
timezone to different timezone")` |
| `adjust_timestamp_to_timezone`, `safe: true` | null |
| `string_to_datetime` date-only site | `err("error computing timezone
offset")` |
| `string_to_datetime` naive-datetime site | `err("error computing timezone
offset")` |
| `string_to_datetime` named-zone site | `err("error computing timezone
offset")` |
The three parser sites return a `ParseError`, which the string cast then
turns into a null under `safe: true` and propagates under `safe: false`. I
confirmed both modes still behave that way, and that genuinely bad input is
unaffected:
```
str safe=false ["2024-03-10 02:30:00", "2024-11-03 01:30:00"] ->
[Some(1710055800), Some(1730615400)]
str safe=true ["2024-03-10 02:30:00", "2024-11-03 01:30:00"] ->
[Some(1710055800), Some(1730615400)]
str safe=false ["not a timestamp"] -> Parser error: ... error parsing date
str safe=true ["not a timestamp"] -> [None]
```
## 4. The known divergence deserves the same prominence it has in #11038
`arrow-array/src/types.rs:349` is untouched. I confirmed that: `git diff`
against the base commit shows this branch changes nothing under `arrow-array/`,
and the function still reads `Ambiguous(dt1, _)` and `None => None`. Measured
in `America/New_York`:
| `2024-11-03 01:30:00` | Result |
| --- | --- |
| `arrow_array::types::from_naive_datetime` | 1730611800 = `05:30Z`, the
**earlier** instant |
| `arrow_cast::local_time::resolve_local_offset` | 1730615400 = `06:30Z`,
the **later** instant |
`2024-03-10 02:30:00` gives `None` from the first and 1710055800 from the
second.
#11038 carries a full table and a paragraph on why the two must be
reconciled. This PR mentions it in one parenthesis inside "What changes are
included". I recommend the fuller disclosure here too. This is the PR that
creates the single shared helper, so it is the PR that makes the surviving
divergence look deliberate.
---
## What I checked and found correct
**Both paths now share one policy — measured, not assumed.** I ran 5,012
boundary readings across 226 zones through the cast path and the string path in
the same run. Every DST transition of every zone from 2020 to 2025, sampled at
the start and the midpoint of each gap and each repeated interval: 2,506 in a
gap, 2,506 ambiguous.
* The cast path and the string path agree on all 5,012 readings.
* 0 unresolved readings under `safe: true`.
That agreement is the central claim of this PR, and it holds across the
whole tzdb, not just the tested zones.
**Against PostgreSQL.** 3,908 of those readings use a zone PostgreSQL 17.11
accepts. 16 disagree, on both paths equally. All 16 are `Europe/Chisinau` and
its alias `Europe/Tiraspol`. That is the chrono-tz tzdata skew #11038
discloses, and it predates both PRs:
```
main Europe/Chisinau 2024-03-31 03:30 -> 1711845000
PostgreSQL 17.11 -> 1711848600
DuckDB 1.5.2 -> 1711848600
```
PostgreSQL and DuckDB agree with each other, so chrono-tz is the outlier.
Please name `Europe/Tiraspol` alongside `Europe/Chisinau`.
**Per-row resolution on the string path.** One `StringArray` that straddles
both 2024 `America/New_York` transitions produces three distinct offsets, and
every element matches PostgreSQL:
| String | arrow | PostgreSQL 17.11 |
| --- | --- | --- |
| `2024-01-15 12:00:00` | 1705338000 | 1705338000 |
| `2024-03-10 01:30:00` | 1710052200 | 1710052200 |
| `2024-03-10 02:30:00` (gap) | 1710055800 | 1710055800 |
| `2024-03-10 03:30:00` | 1710055800 | 1710055800 |
| `2024-11-03 01:30:00` (ambiguous) | 1730615400 | 1730615400 |
| `2024-11-03 02:30:00` | 1730619000 | 1730619000 |
| `2024-12-15 12:00:00` | 1734282000 | 1734282000 |
`Utf8`, `LargeUtf8` and `Utf8View` produce identical results on that array.
**The four hard zones the PR names.** All verified against PostgreSQL
independently:
| Zone | Reading | arrow and PostgreSQL |
| --- | --- | --- |
| `Australia/Lord_Howe` | `2024-10-06 02:15` (30-minute step) | 1728143100 |
| `Pacific/Chatham` | `2024-09-29 03:00` (+12:45 / +13:45) | 1727532900 |
| `Australia/Sydney` | `2024-10-06 02:30` (southern gap) | 1728145800 |
| `America/Sao_Paulo` | `2018-11-04` (midnight does not exist) | 1541300400 |
| `America/Havana` | `2024-11-03` (midnight happens twice) | 1730610000 |
**The site-3 argument holds.** I confirmed the PostgreSQL claim directly.
`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`. So PostgreSQL makes no distinction between the two
spellings, and neither does this PR.
**No call site is missed.** After this PR the only remaining local-time
resolution in the tree outside `local_time.rs` is
`arrow-array/src/types.rs:349`, which is the known divergence.
`arrow-array/src/timezone.rs` only forwards to chrono.
**Suite.** `cargo test -p arrow-cast` passes: 395 lib tests and 13 doc
tests. The doc-test count rises from 12 to 13, which is the new
`string_to_datetime` example.
**The fixed-offset caveat is over-broad in the body.** I measured it.
PostgreSQL uses POSIX west-positive signs only when `'+05:30'` acts as a *zone
name* (`SET TimeZone`, or `AT TIME ZONE`), giving `17:30Z`. With the offset
inside the literal, `timestamptz '2024-01-01 12:00:00 +05:30'` gives `06:30Z`,
which agrees with arrow-rs. DuckDB 1.5.2 rejects the zone-name spelling
outright. Please say "as a zone name".
--
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]
