adriangb commented on PR #25161: URL: https://github.com/apache/datafusion/pull/25161#issuecomment-5624161574
Thanks — acted on two of the three, pushed as [`bda12f1`](https://github.com/apache/datafusion/commit/bda12f107f) and [`7876447`](https://github.com/apache/datafusion/commit/78764476df). **1. Out of range values (valid, fixed).** Correct that this widened a panic's reach: the one-argument form now applies a named-zone offset, and Arrow renders a `Timestamp(Second, Some(tz))` with `chrono`'s `DateTime::naive_local`, which panics when the shifted value leaves `NaiveDateTime`'s range. `from_unixtime` now bounds checks its input and returns an error instead of producing a value that panics later: ``` Execution error: Cannot convert -8334601211039 to a timestamp in timezone "America/New_York" for function from_unixtime: the local date and time is outside the supported range ``` Applied to both the one- and two-argument forms — the two-argument form panicked on `main` too, which is #16594. Notes: - The panic fires when the value is **formatted**, not when it is cast, so the new tests render through `arrow::util::display::ArrayFormatter`; a test that only computes the value passes either way. Removing the check makes all five of them fail. - Timezone naive results keep the full `NaiveDateTime` range (they are never shifted), and values that are out of range in UTC as well are still left to the cast, so no existing error message changes. - The common case is free: values inside the range representable in every zone take a fast path over the array's min/max, so resolving the zone offset per value only happens near the limits. This removes #16594's panic reproducer — `from_unixtime(-8334601211038 - 1, 'America/New_York')` errors instead of panicking, and its other reproducer (`8210266876799 + 1`) keeps its existing cast error. It does **not** close that issue, which asks that all `Int64` values be convertible; that is not achievable while the value is rendered through `chrono::NaiveDateTime`. The root cause is still an unguarded `naive_local()` in the Arrow formatter, reachable with no `from_unixtime` involved (`SELECT arrow_cast(-8334601211039, 'Timestamp(Second, Some("America/New_York"))')`); fixing that properly belongs upstream. **2. Public factory signature (not changed, deliberately).** `make_udf_function_with_config!` is the existing convention for config-aware functions in this module: on `main`, `now` and all five `to_timestamp*` functions already use it, so `datafusion::functions::datetime::now(&ConfigOptions)` is already the public signature and there is no zero-argument shim for it. Giving `from_unixtime` a separately-named factory would make it the odd one out among its siblings. It is a user-facing breaking change though, so I've added it explicitly to the "Are there any user-facing changes?" section, which previously only mentioned the `FromUnixtimeFunc::new()` deprecation. **3. Documentation description (valid, fixed).** The description was wrong on both counts and predates this PR: `from_unixtime` returns an Arrow `Timestamp(Second, ...)`, not an RFC3339 string with nanosecond precision, and the trailing `Z` contradicted the example directly below it, which shows `-04:00`. Rewritten to describe a second-resolution timestamp in the selected time zone, keeping the session time zone paragraph. Docs regenerated with `dev/update_function_docs.sh`. `cargo fmt --all`, `cargo clippy --all-targets -- -D warnings`, `cargo test -p datafusion-functions from_unixtime` and the full `cargo test -p datafusion-sqllogictest --test sqllogictests` (506 files) are green. -- 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]
