adriangb commented on PR #11038:
URL: https://github.com/apache/arrow-rs/pull/11038#issuecomment-5626606546
Self-review QA pass — this is our own PR, so treat this as our own
adversarial check, not an independent review.
No blocking defect. I re-measured every claim. Four findings, in priority
order.
## 1. The stated safety argument for the 24-hour probe is not the
load-bearing one
The PR justifies the probe with a transition-spacing figure: no two
transitions in 597 zones are closer than 167 hours. That figure is correct. I
reproduce it exactly (`America/Boa_Vista` 2000-10-15, 167.00 h). But it is not
the property that makes the probe resolvable.
Two separate invariants are at work here:
* **Transition spacing above 24 hours.** This makes the probed offset the
*correct* pre-transition offset. Margin: 167 hours. The PR states this one.
* **Maximum local gap at or below 24 hours.** This makes the probe land
*outside* the gap at all. The PR does not state this one, and it has no margin.
Seven zones have a local gap of exactly 24.00 hours. These are the dateline
changes:
| Zone | Transition (UTC) | Local gap |
| --- | --- | --- |
| `Pacific/Kwajalein` | 1993-08-21T12:00Z | 24.00 h |
| `Kwajalein` | 1993-08-21T12:00Z | 24.00 h |
| `Pacific/Kiritimati` | 1994-12-31T10:00Z | 24.00 h |
| `Pacific/Kanton` | 1994-12-31T11:00Z | 24.00 h |
| `Pacific/Enderbury` | 1994-12-31T11:00Z | 24.00 h |
| `Pacific/Apia` | 2011-12-30T10:00Z | 24.00 h |
| `Pacific/Fakaofo` | 2011-12-30T11:00Z | 24.00 h |
The next largest gap is 10.00 h (`Antarctica/Macquarie`). So the
distribution is bimodal, and the code sits exactly on the upper mode.
For a reading at the last second of such a gap the probe lands one second
before the gap starts. The real margin is **one second**, not 167 hours. The
code is correct today. It stops being correct the day tzdb gains a gap above 24
hours.
I tested all four surviving dateline zones at the first instant of the gap,
the middle and the last second. All 13 readings match PostgreSQL 17.11 exactly:
```
Pacific/Apia 2011-12-30 00:00:00 -> 1325239200 (PG 1325239200)
Pacific/Apia 2011-12-30 00:00:01 -> 1325239201 (PG 1325239201)
Pacific/Apia 2011-12-30 12:00:00 -> 1325282400 (PG 1325282400)
Pacific/Apia 2011-12-30 23:59:59 -> 1325325599 (PG 1325325599)
Pacific/Kiritimati 1994-12-31 00:00:00 -> 788868000 (PG 788868000)
Pacific/Kwajalein 1993-08-21 23:59:59 -> 746020799 (PG 746020799)
Pacific/Fakaofo 2011-12-30 23:59:59 -> 1325329199 (PG 1325329199)
...
```
Please state the gap-length invariant next to the spacing invariant, both in
the PR body and in the doc comment on `resolve_local_offset`. A future reader
who bumps `chrono-tz` needs to know which of the two numbers to re-check.
## 2. The give-up branch is unreachable, so its callers are untested
`LocalResult::None` from the probe reaches `.earliest()`, which returns
`None`. I could not construct any input that reaches it. Both invariants above
exclude it:
* No gap exceeds 24 hours, so the probe never lands in another gap.
* No fall-back transition sits within 24 hours before a gap, so the probe
never lands in an ambiguous interval either.
So the branch is dead code with real tzdata. I read the callers instead of
testing them, and both are correct:
* `safe: false` — `adjust` returns `None`, `try_unary` raises
`CastError("Cannot cast timezone to different timezone")`. The whole array
fails, as before.
* `safe: true` — `unary_opt` writes a null. As before.
The `?` in `adjust` covers three separate `None` sources: `as_datetime`,
`resolve_local_offset` and `from_naive_datetime`. All three collapse into the
same message. That is pre-existing and fine, but it means the new `None` source
has no distinct diagnostic.
I confirmed the two options stay in step on real DST input:
```
cast safe=false [Some(1710055800), Some(1730615400)]
cast safe=true [Some(1710055800), Some(1730615400)]
```
## 3. The fixed-offset claim is over-broad
The PR says PostgreSQL reads a fixed-offset string such as `'+05:30'` with
the opposite sign. That is true for one spelling only. I measured both on
PostgreSQL 17.11:
| Spelling | Result | Sign convention |
| --- | --- | --- |
| `SET TimeZone='+05:30'` then `'2024-01-01 12:00:00'::timestamptz` |
`17:30Z` | POSIX, west-positive |
| `'2024-01-01 12:00:00'::timestamp AT TIME ZONE '+05:30'` | `17:30Z` |
POSIX, west-positive |
| `timestamptz '2024-01-01 12:00:00 +05:30'` | `06:30Z` | ISO, east-positive
|
So PostgreSQL agrees with arrow-rs when the offset sits inside the literal,
and disagrees when the offset acts as a zone name. DuckDB 1.5.2 rejects the
zone-name spelling outright (`Not implemented Error: Unknown TimeZone
'+05:30'`). Please say "as a zone name" in the PR body. The current wording
reads as though PostgreSQL always disagrees.
## 4. The dev-dependency removes the no-`chrono-tz` coverage of arrow-cast
`arrow-array = { workspace = true, features = ["chrono-tz"] }` under
`[dev-dependencies]` unifies across the graph. So `cargo test -p arrow-cast`
now always builds `Tz` with IANA support, and arrow-cast has no test that
exercises the offset-only `Tz`. That is exactly why
`test_cast_string_to_timestamp_invalid_tz` needed the relaxed assertion.
The relaxed assertion is the right call. I raise this only so the lost
coverage is on the record.
---
## What I checked and found correct
**The per-row claim holds.** This was my main concern, because a
once-per-array offset gives silently wrong values rather than a loud error. I
built one 15-row array that straddles both 2024 `America/New_York` transitions,
and compared every element against PostgreSQL 17.11:
| Wall clock reading | 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-06-15 12:00:00` | 1718467200 | 1718467200 |
| `2024-11-03 00:30:00` | 1730608200 | 1730608200 |
| `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 |
Three distinct offsets appear in one array. A single offset per array cannot
produce this.
**A broad independent sweep agrees.** I generated every DST transition of
every zone from 2020 to 2025, then sampled the start and the midpoint of each
gap and each repeated interval. That gives 5,012 readings across 226 zones:
2,506 in a gap, 2,506 ambiguous. Results:
* 0 unresolved readings under `safe: true`.
* 3,908 of them use a zone PostgreSQL 17.11 accepts. 16 disagree.
* All 16 are `Europe/Chisinau` and its alias `Europe/Tiraspol`.
That is the chrono-tz tzdata skew the PR already discloses. I confirmed it
predates this PR. On the base commit, with no change applied:
```
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. It
puts the Moldova spring transition at 02:00 local; the other two put it at
03:00 local. Non-boundary readings agree everywhere. Please add
`Europe/Tiraspol` to the disclosure — it carries the same skew and the PR names
only `Europe/Chisinau`.
**The known divergence stays untouched and disclosed.** `git diff` against
the base commit shows this branch changes nothing under `arrow-array/`.
`arrow-array/src/types.rs:349` still reads `Ambiguous(dt1, _)` and `None =>
None`. Measured side by side in `America/New_York`:
| `2024-11-03 01:30:00` | Result |
| --- | --- |
| `from_naive_datetime` | 1730611800 = `05:30Z`, the **earlier** instant |
| this PR's cast | 1730615400 = `06:30Z`, the **later** instant |
Opposite, as the PR states. The disclosure in the body is accurate and
prominent.
**Test coverage.** The six new tests cover the southern-hemisphere gap
(`Australia/Sydney`), both `safe` modes, the nanosecond path, a unit change and
a fixed offset. `Australia/Lord_Howe`, `Pacific/Chatham`, `America/Sao_Paulo`
and `America/Havana` are not in this PR's tests, but #11054 adds them. I
verified all four against PostgreSQL by hand and they pass on this branch too.
**Suite.** `cargo test -p arrow-cast` passes on this branch.
--
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]
