fix: from_unixtime should respect datafusion.execution.time_zone - #25161
fix: from_unixtime should respect datafusion.execution.time_zone#25161adriangb wants to merge 6 commits into
from_unixtime should respect datafusion.execution.time_zone#25161Conversation
`from_unixtime(expr)` (the single argument form) hardcoded `Timestamp(Second, None)`, so it ignored the session time zone while `now()` and the `to_timestamp*` family in the same session correctly returned timestamps in `datafusion.execution.time_zone`: ```sql SET datafusion.execution.time_zone = 'America/Denver'; SELECT arrow_typeof(from_unixtime(1704110400)), from_unixtime(1704110400); -- Timestamp(s) | 2024-01-01T12:00:00 <-- timezone naive ``` Give `FromUnixtimeFunc` the same `timezone` field + `new_with_config()` / `with_updated_config()` treatment that `NowFunc` and the `to_timestamp*` functions already use, and register it with `make_udf_function_with_config!` so the session config reaches it both at registration time and on `SET`/`RESET`. The two argument form `from_unixtime(expr, 'tz')` is unchanged: an explicit timezone still wins over the session time zone. With the session time zone unset (the default) the result is still `Timestamp(Second, None)`, so existing behaviour is preserved. `to_unixtime` is not affected: it returns `Int64` epoch seconds, and it already threads `config_options` into `ToTimestampSecondsFunc` for its string parsing path. `FromUnixtimeFunc::new()` is deprecated in favour of `new_with_config()`, mirroring what was done for the `to_timestamp*` constructors. Closes apache#12892 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| } | ||
|
|
||
| impl FromUnixtimeFunc { | ||
| #[deprecated(since = "55.0.0", note = "use `new_with_config` instead")] |
There was a problem hiding this comment.
55.0.0 is realeased, this should be deprecated in 56.0.0
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
55.0.0 is already tagged and released, so a new deprecation on main belongs in the next release. Main already carries three other `since = "56.0.0"` deprecations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@kosiew relatively small timezone support PR for review 🙏🏻 |
There was a problem hiding this comment.
🟡 Changes recommended
It broadens a known process panic and introduces an unannounced breaking Rust factory signature.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates from_unixtime to honor the session execution timezone while preserving explicit timezone precedence.
Changes:
- Adds configuration-aware timezone handling and registration.
- Adds unit and SQL logic tests for timezone behavior.
- Updates generated function documentation.
File summaries
| File | Description |
|---|---|
docs/source/user-guide/sql/scalar_functions.md |
Documents session timezone behavior. |
datafusion/sqllogictest/test_files/from_unixtime_timezone.slt |
Tests timezone and reset scenarios. |
datafusion/sql/src/unparser/expr.rs |
Uses the non-deprecated default constructor. |
datafusion/functions/src/datetime/mod.rs |
Registers and exports the config-aware UDF. |
datafusion/functions/src/datetime/from_unixtime.rs |
Implements timezone-aware results and unit tests. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| match len { | ||
| 1 => args[0].cast_to(&Timestamp(Second, None), None), | ||
| 1 => args[0].cast_to(&Timestamp(Second, self.timezone.clone()), None), |
| make_udf_function!(to_local_time::ToLocalTimeFunc, to_local_time); | ||
| make_udf_function!(to_time::ToTimeFunc, to_time); | ||
| make_udf_function!(to_unixtime::ToUnixtimeFunc, to_unixtime); | ||
| make_udf_function_with_config!(from_unixtime::FromUnixtimeFunc, from_unixtime); |
| description = r#" | ||
| Converts an integer to RFC3339 timestamp format (`YYYY-MM-DDT00:00:00.000000000Z`). | ||
| Integers and unsigned integers are interpreted as seconds since the unix epoch | ||
| (`1970-01-01T00:00:00Z`) return the corresponding timestamp. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25161 +/- ##
==========================================
+ Coverage 81.60% 81.92% +0.32%
==========================================
Files 1123 1132 +9
Lines 408898 421358 +12460
Branches 408898 421358 +12460
==========================================
+ Hits 333670 345188 +11518
- Misses 55625 55779 +154
- Partials 19603 20391 +788 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The description predates this PR and was wrong in both halves: `from_unixtime` returns an Arrow `Timestamp(Second, ...)`, not an RFC3339 string with nanosecond precision, and the trailing `Z` contradicted the example right below it, which shows a `-04:00` offset. Describe what the function actually returns and regenerate `docs/source/user-guide/sql/scalar_functions.md`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…f range
A `Timestamp(Second, Some(tz))` is *rendered* by shifting the UTC instant by the
zone's offset, and Arrow does that with `chrono`'s `DateTime::naive_local`, which
panics when the shifted value falls outside `NaiveDateTime`'s range:
SELECT from_unixtime(-8334601211039, 'America/New_York');
-- thread 'main' panicked: Local time out of range for `NaiveDateTime`
The panic fires when the value is *formatted*, not when it is produced, so it
surfaces far away from `from_unixtime`.
The two argument form has always been able to reach it
(apache#16594). The previous commits on this
branch let the one argument form reach it too, because it now applies the session
time zone.
Check the bound in `from_unixtime` and return a `DataFusionError` instead of
producing a value that panics downstream. Values inside the range that is
representable in every time zone (the overwhelmingly common case) take a fast
path that only looks at the array's min and max, so the per-value offset
resolution is confined to inputs near the limits.
Timezone naive results are never shifted and keep the full `NaiveDateTime`
range, exactly as before. Values that are out of range in UTC as well are left
to the cast, which already reports them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — acted on two of the three, pushed as 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 Applied to both the one- and two-argument forms — the two-argument form panicked on
This removes #16594's panic reproducer — 2. Public factory signature (not changed, deliberately). 3. Documentation description (valid, fixed). The description was wrong on both counts and predates this PR:
|
`unparseable` -> `unparsable`, in a comment added by the previous commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Minor merge-order note: #25175 (timezone characterization tests) has a No assertion in it breaks — that block runs with Whichever merges second should update the comment. Flagging it so the stale text does not survive review. |
|
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 Findings1. The description states "No previously-successful query changes behaviour". That is false.Two queries succeed on 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 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 2.
|
| 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 #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 runsminandmax): 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 passcargo test -p datafusion-sqllogictest --test sqllogictests— 506 files passcargo clippy -p datafusion-functions -p datafusion-sql --all-targets -- -D warnings— cleancargo fmt --all --check— clean./ci/scripts/doc_prettier_check.sh— clean
What I did not verify
- Serialization.
FromUnixtimeFuncnow 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.nowandto_timestamp*behave the same way onmain, so this is not new, but I did not run a proto round trip. - Behaviour across a
SETinside a prepared statement or a view. - Any engine other than
datafusion-cli.
|
Correction to my own review comment above, item 3: Items 1, 2, 4 and 5 in that comment stand. Item 4 in particular — |
`mod.rs` still described `from_unixtime` as converting "an integer to RFC3339 timestamp format string". The function returns an Arrow `Timestamp(Second, ...)`, not a string, and not at nanosecond resolution. This is the same wrong claim that an earlier commit corrected in the `user_doc` block. The `expr_fn` copy was missed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| statement ok | ||
| SET datafusion.execution.time_zone = 'America/Denver'; | ||
|
|
||
| query TP |
There was a problem hiding this comment.
Could we add an America/Denver summer-instant assertion here, for example using a date in July that renders with -06:00? The named-zone coverage currently only checks January at -07:00, so adding a summer case would make sure we are testing DST behavior rather than something that could also be satisfied by a fixed offset. This is non-blocking since Arrow already provides the timezone rendering behavior, but it would help make the SQL contract a bit more robust.
Which issue does this PR close?
Rationale for this change
The bug, in SQL
Set a session time zone. Every other date and time function honours it.
from_unixtimedoes not.The last row is the defect. The user asks for a Denver session, but the value comes back as a bare wall clock with no zone.
2024-01-01T12:00:00is the UTC clock, not the Denver clock.After this PR the same query returns the session time zone:
The instant is the same. Only the declared type and the rendered offset change.
What changes for a user
from_unixtime(expr)returns a value indatafusion.execution.time_zone.from_unixtime(expr, 'tz')is unchanged. An explicit argument still wins.Field research: PostgreSQL and DuckDB
Neither engine has a function named
from_unixtime. The closest equivalent in both isto_timestamp(<epoch>), so that is the comparison I ran. Both engines are timezone-aware there, and both render in the session time zone.PostgreSQL 17.11
PostgreSQL has no timezone-naive form of
to_timestamp. A user must ask for one:DuckDB v1.5.2
DuckDB defaults its session time zone to the operating system zone, so it is never naive. The naive form is
make_timestamp(<microseconds>), which returnsTIMESTAMP.What the comparison supports, and what it does not
from_unixtimestays naive out of the box. PostgreSQL and DuckDB always have a session zone.SET TimeZone = '+08:00'in PostgreSQL means UTC-08:00, because PostgreSQL reads the string POSIX-style. DataFusion reads'+08:00'as UTC+08:00. That divergence is separate and is tracked inAT TIME ZONE '+05:30'uses the opposite sign convention from PostgreSQL #25170.ERROR: timestamp out of rangeatto_timestamp(-8334601211039). DuckDB renders the same value as262145-12-31 (BC) 23:59:59.000448-04:56. DataFusion sits between the two, and this PR makes it raise an error rather than panic.What changes are included in this PR?
The fix
FromUnixtimeFunccarries anOption<Arc<str>> timezonetaken fromconfig.execution.time_zone, and implementsScalarUDFImpl::with_updated_config.NowFuncand theto_timestamp*functions already work this way. The single-argument form reports and producesTimestamp(Second, <session tz>)from bothreturn_field_from_argsandinvoke_with_args.from_unixtimemoves tomake_udf_function_with_config!, and itsexpr_fngains the@configmarker. The sessionConfigOptionsnow reach the function at registration time, and again on everySETorRESET.FromUnixtimeFunc::new()is deprecated since56.0.0in favour ofFromUnixtimeFunc::new_with_config(). This mirrors theto_timestamp*constructors.Defaultstill yields the previous timezone-naive behaviour.from_unixtimedocumentation description was wrong before this PR. It claimed an RFC3339 string with nanosecond precision and aZsuffix, but the function returns an ArrowTimestamp(Second, ...), and the example directly below it shows a-04:00offset. The description now states what the function returns, anddocs/source/user-guide/sql/scalar_functions.mdis regenerated withdev/update_function_docs.sh.The guard
Arrow renders a
Timestamp(Second, Some(tz))throughchrono'sDateTime::naive_local, which panics when the shifted value leavesNaiveDateTime's range. The panic fires at render time, far fromfrom_unixtime. The two-argument form can always reach it. Once the one-argument form applies a session zone, it can reach it too.So
invoke_with_argsnow checks the bound and returns aDataFusionError:The check has three parts:
NaiveDateTimerange stays usable, exactly as onmain.minandmax. Inside[NaiveDateTime::MIN + 86400, NaiveDateTime::MAX - 86400]no zone offset can push a value out of range, so no per-value work happens.86400is a deliberate over-bound. The largest offset in the whole IANA database isAsia/Manilaat-15:56:08local mean time, and the largest positive one isAmerica/Metlakatlaat+15:13:42. The largest fixed offset Arrow'sTzparser accepts is+23:59, which is86340seconds.+24:00is rejected by the parser.What is the testing strategy for this PR?
New sqllogictest file
datafusion/sqllogictest/test_files/from_unixtime_timezone.slt, modelled onto_timestamp_timezone.slt. It asserts botharrow_typeofand the value for:+08:00),America/Denver),NULLinput,RESET, which must restore the timezone-naive behaviour.The
to_unixtime/from_unixtimeround trip from the issue is in the file. So are the out-of-range bounds, in both directions, for a named zone and a fixed offset zone, for the one-argument and the two-argument forms, and for array input.New unit tests in
datafusion/functions/src/datetime/from_unixtime.rscoverreturn_field_from_argsandinvoke_with_argswith a session time zone set, an explicit time zone that overrides it, and the out-of-range bound.Every bounds test renders its result. The unit tests go through
arrow::util::display::ArrayFormatter, and the sqllogictest cases usequery P. This is deliberate: the panic they guard against fires when the value is formatted, not when it is produced, so a test that only computes the value would pass either way.Verified locally
cargo test -p datafusion-sqllogictest --test sqllogictests— 506 files passcargo test -p datafusion-functions --lib datetime::from_unixtime— 10 tests passcargo clippy -p datafusion-functions -p datafusion-sql --all-targets -- -D warnings— cleancargo fmt --all --checkand./ci/scripts/doc_prettier_check.sh— cleanVerified against a
mainbuild. I builtmainat the merge base and this branch as twodatafusion-clibinaries and diffed their output. With the session time zone unset, eleven probes are byte-identical, including the error paths. The two-argument form is byte-identical across 140 pairs of value and zone. The full detail is in a separate comment on this PR.Cost of the guard.
SELECT count(from_unixtime(v)) FROM generate_series(1, 100000000) t(v), three runs each,ciprofile: 3.95 s / 3.65 s / 3.07 s with the zone unset, and 3.09 s / 3.78 s / 3.33 s withAmerica/Denver. The two extra kernel passes sit inside the noise.Are there any user-facing changes?
Yes, four.
1. The behaviour change, which is the bug fix. When
datafusion.execution.time_zoneis set,from_unixtime(expr)returnsTimestamp(Second, <session tz>)instead ofTimestamp(Second, None). The epoch value is unchanged. Only the declared type and the rendered offset differ. With the default unset session time zone nothing changes.2. An API deprecation.
FromUnixtimeFunc::new()is deprecated since56.0.0in favour ofFromUnixtimeFunc::new_with_config().Default::default()stays available and behaves like the oldnew().3. A public factory signature change.
datafusion::functions::datetime::from_unixtime()becomesfrom_unixtime(&ConfigOptions), because it now usesmake_udf_function_with_config!. This breaks callers of that factory. It is the established convention for config-aware functions in this module:nowand all fiveto_timestamp*functions already have this signature onmain, with no zero-argument shim. Theexpr_fnwrapperdatafusion::functions::expr_fn::from_unixtime(expr)keeps its signature.4. A new error in place of a panic. See below.
Out of range values
Arrow renders a
Timestamp(Second, Some(tz))value by a shift of the UTC instant by the zone's offset. It does that withchrono'sDateTime::naive_local, which panics when the shifted value leavesNaiveDateTime's range. The panic fires when the value is formatted, not when it is cast, so it surfaces far from where it starts:This PR replaces that panic with an error. The check applies to both forms. Its deliberate limits:
NaiveDateTimerange stays usable, exactly as onmain.Cast error: Failed to convert 8210266876800 to datetime for ...). No error message changes.mainsuch a value is only fatal at render time, soSELECT to_unixtime(from_unixtime(-8334601211039, 'America/New_York'))returns-8334601211039today and errors after this PR. Every query that produced a rendered result keeps its behaviour.Known interaction with issue 16594
This removes the panic reproducer in #16594, which I verified directly.
from_unixtime(-8334601211038 - 1, 'America/New_York')errors instead of a panic, andfrom_unixtime(8210266876799 + 1, 'America/New_York')keeps its existing cast error.It does not close that issue. The issue asks that all
Int64values convert, and that is not reachable whilechrono::NaiveDateTimerenders the value.The underlying defect is an unguarded
naive_local()in the Arrow timestamp-with-timezone formatter. It stays reachable with nofrom_unixtimeat all:A proper guard belongs in Arrow. The check here only stops
from_unixtimefrom handing the formatter a value the formatter cannot render.Merge order with PR 25175
#25175 pins today's
from_unixtimebehaviour in a characterization test file. No assertion in it breaks. Its SECTION 10 cases run with the session time zone unset, and this PR keepsTimestamp(Second, None)in that case. I replayed both assertions on this branch and they still pass:The comment above them says
from_unixtime"ignoresdatafusion.execution.time_zone". That comment goes stale when this PR lands, so whichever merges second must reword it.🤖 Generated with Claude Code