adriangb opened a new pull request, #25161: URL: https://github.com/apache/datafusion/pull/25161
## Which issue does this PR close? - Closes https://github.com/apache/datafusion/issues/12892 ## Rationale for this change `from_unixtime(expr)` ignored `datafusion.execution.time_zone`, while `now()` and the `to_timestamp*` family in the same session honoured it. That is inconsistent, and it makes the round trip through `to_unixtime` surprising: ```sql SET datafusion.execution.time_zone = 'America/Denver'; SELECT arrow_typeof(now()), arrow_typeof(to_timestamp(1704110400)); -- Timestamp(ns, "America/Denver") | Timestamp(ns, "America/Denver") SELECT arrow_typeof(from_unixtime(1704110400)), from_unixtime(1704110400); -- Timestamp(s) | 2024-01-01T12:00:00 <-- timezone naive, session tz ignored ``` After this PR the last query returns `Timestamp(s, "America/Denver")` and `2024-01-01T05:00:00-07:00`, i.e. the same instant rendered in the session time zone. ## What changes are included in this PR? - `FromUnixtimeFunc` now carries an `Option<Arc<str>> timezone` taken from `config.execution.time_zone`, and implements `ScalarUDFImpl::with_updated_config`, exactly like `NowFunc` and the `to_timestamp*` functions already do. The single-argument form reports and produces `Timestamp(Second, <session tz>)` from both `return_field_from_args` and `invoke_with_args`. - `from_unixtime` is registered with `make_udf_function_with_config!` (and exported as an `@config` `expr_fn`) so the session `ConfigOptions` reach it at registration time and again whenever the config changes via `SET` / `RESET`. - `FromUnixtimeFunc::new()` is deprecated in favour of `FromUnixtimeFunc::new_with_config()`, mirroring the `to_timestamp*` constructors. `Default` still yields the previous (timezone-naive) behaviour. - Documentation for `from_unixtime` updated (and `docs/source/user-guide/sql/scalar_functions.md` regenerated with `dev/update_function_docs.sh`). Deliberately unchanged: - **The two-argument form `from_unixtime(expr, 'tz')`** keeps using the explicit timezone; an explicit argument wins over the session time zone. - **With the session time zone unset (the default) behaviour is byte-for-byte what it was**: `Timestamp(Second, None)`. - **`to_unixtime` has no symmetric defect.** It returns `Int64` epoch seconds, which is timezone-independent, and it already threads `args.config_options` into `ToTimestampSecondsFunc` for its string-parsing path. It is covered by a round-trip test here but is otherwise untouched. ## What is the testing strategy for this PR? - New sqllogictest file `datafusion/sqllogictest/test_files/from_unixtime_timezone.slt`, modelled on `to_timestamp_timezone.slt`. It asserts both `arrow_typeof` and the value for: session time zone unset; a fixed offset (`+08:00`); a named IANA zone (`America/Denver`); the explicit two-argument form under a set session time zone (must ignore it); array (non-constant-folded) input; `NULL` input; equality of the same instant across two timezones; and `RESET` restoring the timezone-naive behaviour. The `to_unixtime` / `from_unixtime` round trip from the issue is included. - New unit tests in `datafusion/functions/src/datetime/from_unixtime.rs` covering `return_field_from_args` + `invoke_with_args` with a session time zone set, and the explicit timezone argument overriding it. - Full `cargo test -p datafusion-sqllogictest --test sqllogictests` suite passes, as do `cargo test -p datafusion-functions -p datafusion-sql --lib`, `cargo fmt --all --check`, and `cargo clippy --all-targets -- -D warnings`. ## Are there any user-facing changes? Yes, two: 1. **Behaviour change (the bug fix).** When `datafusion.execution.time_zone` is set, `from_unixtime(expr)` now returns `Timestamp(Second, <session tz>)` instead of `Timestamp(Second, None)`. The underlying epoch value is unchanged - only the declared type and the rendered offset differ. With the default (unset) session time zone nothing changes. 2. **API deprecation.** `FromUnixtimeFunc::new()` is deprecated in favour of `FromUnixtimeFunc::new_with_config()`; `Default::default()` remains available and behaves like the old `new()`. 🤖 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
