adriangb commented on PR #25161:
URL: https://github.com/apache/datafusion/pull/25161#issuecomment-5626553910
**Self-review. This is my own PR, so this is a QA pass on my own work, not
an independent review.** Its value is the list of things I ran, and the risk
that remains after them.
I built `main` (`da89c7c85b`, the exact merge base) and this branch
(`e8cb6a69`) as two `datafusion-cli` binaries, and I compared them directly.
Everything below comes from a run, not from a read.
---
## Findings
### 1. The description states "No previously-successful query changes
behaviour". That is false.
Two queries succeed on `main` and fail on this branch:
```sql
SELECT to_unixtime(from_unixtime(-8334601211039, 'America/New_York'));
-- main: -8334601211039
-- branch: Execution error: Cannot convert -8334601211039 to a timestamp in
timezone
-- "America/New_York" for function from_unixtime: ...
SELECT from_unixtime(-8334601211039, 'America/New_York')
> from_unixtime(0, 'America/New_York');
-- main: false
-- branch: Execution error: ... (same)
```
The reason is a real asymmetry. On `main` the value is only fatal when a
formatter renders it. A query that computes with the value but never renders it
works today. The new guard rejects the value earlier, at production time, so
those queries now fail.
The guard is still the right trade: the alternative is a process panic. But
the sentence must go. A correct statement is: *no query that produced a
rendered result changes behaviour; a query that consumed an out-of-range value
without rendering it now returns an error.*
A `count(*)` over the same expression still works on both binaries, because
the projection is pruned before `invoke_with_args` runs.
### 2. `to_timestamp_seconds` still panics on the same input, on this branch.
```sql
SET datafusion.execution.time_zone = 'America/New_York';
SELECT to_timestamp_seconds(-8334601211039);
-- main: thread 'main' panicked ... Local time out of range for
`NaiveDateTime`
-- branch: thread 'main' panicked ... Local time out of range for
`NaiveDateTime`
```
This is a sibling in the same module. It returns `Timestamp(Second, <session
tz>)`, which is the exact shape the guard protects in `from_unixtime`. It is
pre-existing and this PR does not make it worse, but the description names only
`arrow_cast` as the residual path. `to_timestamp_seconds` is a much more likely
route for a user, so it belongs in the residual-risk list. The "removes the
panic reproducer in #16594" line reads wider than the change is.
### 3. `cargo-semver-checks` is red on this PR and the description does not
say so.
The bot reports `function_parameter_count_changed` for
`datafusion_functions::datetime::from_unixtime` and
`type_method_marked_deprecated` for `FromUnixtimeFunc::new`. The description
argues the convention (`now` and the five `to_timestamp*` factories already
take `&ConfigOptions`), which is correct, but a reviewer must not discover the
red bot on their own.
### 4. The `expr_fn` doc string keeps the wrong text that commit `bda12f1`
fixed elsewhere.
`datafusion/functions/src/datetime/mod.rs:82` still reads:
```rust
from_unixtime,
"converts an integer to RFC3339 timestamp format string",
@config unixtime
```
That is the same claim the `user_doc` change removed. A one-line follow-up.
### 5. Nit: the `#[deprecated]` attribute sits above the `///` block on
`new()`.
Convention in this repo puts the doc comment first and the attribute after
it.
---
## What I verified, and how
### The `86400` over-bound is safe, with margin
I enumerated all 598 IANA zones and took the UTC offset at year 1 (the local
mean time era, before any transition) and at year 9999 (the POSIX footer rule).
The two extremes are:
- `Asia/Manila`, LMT `-15:56:08` = `-57368` s
- `America/Metlakatla`, LMT `+15:13:42` = `+54822` s
Both are inside `86400`, so the fast path leaves about 29000 s of headroom
for named zones. I then confirmed `chrono-tz` agrees with that data at the
exact boundary:
```sql
SELECT from_unixtime(-8334601171432, 'Asia/Manila'); --
-262143-01-01T00:00:00-15:56
SELECT from_unixtime(-8334601171433, 'Asia/Manila'); -- Execution error, as
intended
```
Fixed offsets are the tighter case. `+23:59` is `86340` s, which passes:
```sql
SET datafusion.execution.time_zone = '+23:59';
SELECT from_unixtime(8210266790459); -- +262142-12-31T23:59:59+23:59
```
`+24:00` and `+99:00` never reach the guard, because Arrow's `Tz` parser
rejects them first (`Invalid timezone "+24:00": failed to parse timezone`). So
`86400` is a true over-bound for every zone the engine can hold.
### The guard's boundary is exact, and the tests are real
The counterfactual in the description is "removing the bounds check makes
all five of them fail". I did not take that on trust. I ran the same values
through `arrow_cast`, which is the unguarded path, in the same binary:
```sql
SELECT arrow_cast(8210266844399, 'Timestamp(Second, Some("Asia/Tokyo"))');
-- renders
SELECT arrow_cast(8210266844400, 'Timestamp(Second, Some("Asia/Tokyo"))');
-- panics
```
The guard errors at exactly `8210266844400` and passes `8210266844399`. So
the bound is neither loose nor tight by one second.
On the "do the tests render?" question: yes. The unit tests call
`display()`, which builds an `arrow::util::display::ArrayFormatter`. The
sqllogictest cases use `query P`, which renders the value. A test that only
computed the value would prove nothing here, and neither kind of test does that.
### The two-argument form is unchanged
I ran 140 pairs — 20 values by 7 zones — through both binaries and diffed
the output. The zones include `Australia/Lord_Howe` (a 30-minute DST step) and
`Pacific/Chatham` (`+12:45`). The values include `0`, `±1`, the `NaiveDateTime`
extremes, and 16 random values across `±4e12`. Result: byte-identical.
### The default (unset) session zone is byte-identical to `main`
Eleven probes, including the error paths, all byte-identical:
| Probe | Result |
| --- | --- |
| `arrow_typeof` + value, scalar | identical |
| `NULL` input | identical |
| Two-argument form | identical |
| Both `NaiveDateTime` extremes | identical |
| Array (not constant folded) input | identical |
| `to_unixtime(from_unixtime(...))` | identical |
| Empty timezone string `''` | identical |
| Invalid zone `'Not/AZone'` | identical |
| Wrong argument type | identical |
| Equality against an explicit `'UTC'` | identical |
| `arrow_cast`-wrapped input | identical |
### The documentation examples reproduce exactly
Both blocks in `scalar_functions.md` match the real output character for
character, offsets included.
### The interaction with #25175 holds
I replayed SECTION 10's two `from_unixtime` assertions from
https://github.com/apache/datafusion/pull/25175 on this branch, with the
session zone unset:
```
Timestamp(s) 2024-07-01T00:00:00
Timestamp(s, "America/Denver") 2024-06-30T18:00:00-06:00
```
Both still pass. Only the comment above them goes stale, exactly as #25175
says.
### The guard costs nothing measurable
`SELECT count(from_unixtime(v)) FROM generate_series(1, 100000000) t(v)`,
three runs each, `ci` profile:
- session zone unset (guard short-circuits): 3.95 s, 3.65 s, 3.07 s
- session zone `America/Denver` (guard runs `min` and `max`): 3.09 s, 3.78
s, 3.33 s
The two extra kernel passes sit inside the run-to-run noise. The "the common
case is free" claim holds at this scale.
### Local checks
- `cargo test -p datafusion-functions --lib datetime::from_unixtime` — 10
tests pass
- `cargo test -p datafusion-sqllogictest --test sqllogictests` — 506 files
pass
- `cargo clippy -p datafusion-functions -p datafusion-sql --all-targets --
-D warnings` — clean
- `cargo fmt --all --check` — clean
- `./ci/scripts/doc_prettier_check.sh` — clean
---
## What I did not verify
- Serialization. `FromUnixtimeFunc` now carries state, so a plan that a
session with a zone builds, and a session without a zone decodes, resolves the
function by name and picks up the second session's zone. `now` and
`to_timestamp*` behave the same way on `main`, so this is not new, but I did
not run a proto round trip.
- Behaviour across a `SET` inside a prepared statement or a view.
- Any engine other than `datafusion-cli`.
--
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]
