You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "KassaSana (via GitHub)" <gi...@apache.org> on 2026/09/27 03:56:47 UTC
[PR] fix: Validate `execution.time_zone` at `SET` time [datafusion]
KassaSana opened a new pull request, #25788:
URL: https://github.com/apache/datafusion/pull/25788
## Which issue does this PR close?
- Part of #17498. This covers `datafusion.execution.time_zone` only.
## Rationale for this change
`datafusion.execution.time_zone` is stored as a plain string, so an invalid value such as `SET TIME ZONE = 'Asia/Taipei2'` (or `'08:00'`, `'+08:00:00'`) is accepted and shown by `SHOW TIME ZONE`. The error only appears later, on the first `::TIMESTAMPTZ` cast. Worse, `current_date()` and `current_time()` parse the value with `.ok()`, so they silently ignore a bad time zone and return results as if none was set.
PostgreSQL and Spark (`spark.sql.session.timeZone` is checked with `isValidTimezone` in `SQLConf`) both reject an invalid time zone when it is set.
This picks up #23224 by @Probablism, which was closed as stale, and answers its open review questions:
- **Normalization** (the `timestamps.slt` question): #23224 stored a parsed `Tz`, so `+08` came back as `+08:00` and `arrow_typeof(now())` changed. Here the value is checked with `Tz::from_str` but stored exactly as written, so the only change users see is that invalid values are rejected. `timestamps.slt` is unchanged, and `set_variable.slt` now checks that `SHOW TIME ZONE` returns `+08`.
- **`AEST` in tests**: `AEST` was never a valid Arrow time zone. The tests that used it only passed the string through without parsing it, so they now use `Australia/Sydney`.
## What changes are included in this PR?
- Add `ConfigTimeZone` in `datafusion/common/src/config.rs`. It can only be built through `FromStr`, which checks the value with Arrow's `Tz` parser and keeps the original string. It provides `as_str()` and `Display`.
- Change `ExecutionOptions::time_zone` from `Option<String>` to `Option<ConfigTimeZone>`. Its `ConfigField` impl parses before assigning, following `DFParquetStatistics`, so a rejected value leaves the previous value (or `None`) in place, and `RESET` restores `None`.
- Update the readers (SQL planner, `now`, `current_date`, `current_time`, `to_timestamp*`, `from_unixtime`, Spark `cast` / `date_trunc`) to use `as_str()`.
- Remove `to_timestamp_invalid_execution_timezone_behavior` and `to_timestamp_formats_invalid_execution_timezone_behavior`, along with their now-unused helper. They put an invalid string straight into the config to test the error at invoke time, and that state can no longer be constructed. The unit test and slt cases below cover the rejection instead.
## What is the testing strategy for this PR?
- `set_variable.slt`: the invalid values `+08:00:00`, `08:00`, `08` and `Asia/Taipei2` now fail on `SET` (through both `SET TIME ZONE` and `SET datafusion.execution.time_zone`), and `SHOW TIME ZONE` confirms the previous value is kept. It also checks that `+08` is shown as written.
- `test_execution_time_zone_validation` in `datafusion-common`: valid values round-trip exactly; invalid values error and keep the previous value; an invalid update of an unset value leaves it unset; nested keys are rejected.
- Existing time zone suites pass unchanged (`timestamps.slt`, `timestamps_timezone.slt`, `current_*_timezone.slt`, `to_timestamp_timezone.slt`, `from_unixtime_timezone.slt`, Spark cast/date_trunc).
Commands run (all passing):
- `cargo test -p datafusion-common --lib config::tests` (with and without `--features parquet`)
- `cargo test --profile=ci --test sqllogictests` (all 523 files)
- `cargo test -p datafusion-functions --lib datetime`
- `cargo test -p datafusion-spark --lib function::`
- `cargo test -p datafusion-sql --lib`
- `cargo test -p datafusion --test user_defined_integration config_options`
- `cargo test -p datafusion-ffi --features integration-tests --test ffi_udf`
- `cargo test -p datafusion --test parquet_integration`
- `cargo fmt --all -- --check`
- `cargo clippy --all-targets --all-features -- -D warnings`
## Are there any user-facing changes?
Yes. `SET TIME ZONE` / `SET datafusion.execution.time_zone` now fails immediately on an invalid time zone instead of failing on a later query, or being silently ignored by `current_date()` / `current_time()`.
This changes the public field type `ExecutionOptions::time_zone` from `Option<String>` to `Option<ConfigTimeZone>`. Code that reads it can use `.as_ref().map(ConfigTimeZone::as_str)`, and code that sets it can use `Some("UTC".parse()?)`. Could a maintainer please add the `api change` label? I'm happy to add a note to the 56.0.0 upgrade guide if that's wanted.
cc @Jefffrey (you reviewed #23224), @kumarUjjawal @alamb (reviewed the other #17498 slices)
🤖 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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Validate `execution.time_zone` at `SET` time [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal merged PR #25788:
URL: https://github.com/apache/datafusion/pull/25788
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Validate `execution.time_zone` at `SET` time [datafusion]
Posted by "subhramit (via GitHub)" <gi...@apache.org>.
subhramit commented on PR #25788:
URL: https://github.com/apache/datafusion/pull/25788#issuecomment-5853486279
https://dontpastetheai.com/ may be useful for you (checked your prior GitHub contributions as well)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Validate `execution.time_zone` at `SET` time [datafusion]
Posted by "KassaSana (via GitHub)" <gi...@apache.org>.
KassaSana commented on PR #25788:
URL: https://github.com/apache/datafusion/pull/25788#issuecomment-5856656575
Thanks for the review @kumarUjjawal! Added the 56.0.0 upgrade guide entry in 0e4254db7.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Validate `execution.time_zone` at `SET` time [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25788:
URL: https://github.com/apache/datafusion/pull/25788#issuecomment-5856293512
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25788?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `97.14286%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.49%. Comparing base ([`acf5c89`](https://app.codecov.io/gh/apache/datafusion/commit/acf5c89e6454469c763c0fb5539e5a294d3439ed?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`84a92d3`](https://app.codecov.io/gh/apache/datafusion/commit/84a92d35ae46bda30313ef8a59d7b19f0d4b65cf?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 9 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25788?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/common/src/config.rs](https://app.codecov.io/gh/apache/datafusion/pull/25788?src=pr&el=tree&filepath=datafusion%2Fcommon%2Fsrc%2Fconfig.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9jb21tb24vc3JjL2NvbmZpZy5ycw==) | 96.15% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25788?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25788 +/- ##
==========================================
- Coverage 82.50% 82.49% -0.01%
==========================================
Files 1141 1141
Lines 438825 438785 -40
Branches 438825 438785 -40
==========================================
- Hits 362050 361995 -55
- Misses 54892 54900 +8
- Partials 21883 21890 +7
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25788?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Validate `execution.time_zone` at `SET` time [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25788:
URL: https://github.com/apache/datafusion/pull/25788#issuecomment-5856220817
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/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>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion v55.1.0 (current)
Built [ 56.224s] (current)
Parsing datafusion v55.1.0 (current)
Parsed [ 0.034s] (current)
Building datafusion v55.1.0 (baseline)
Built [ 56.417s] (baseline)
Parsing datafusion v55.1.0 (baseline)
Parsed [ 0.034s] (baseline)
Checking datafusion v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.775s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 115.937s] datafusion
Building datafusion-common v55.1.0 (current)
Built [ 31.320s] (current)
Parsing datafusion-common v55.1.0 (current)
Parsed [ 0.059s] (current)
Building datafusion-common v55.1.0 (baseline)
Built [ 31.272s] (baseline)
Parsing datafusion-common v55.1.0 (baseline)
Parsed [ 0.061s] (baseline)
Checking datafusion-common v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.890s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 64.798s] datafusion-common
Building datafusion-ffi v55.1.0 (current)
Built [ 53.499s] (current)
Parsing datafusion-ffi v55.1.0 (current)
Parsed [ 0.058s] (current)
Building datafusion-ffi v55.1.0 (baseline)
Built [ 53.577s] (baseline)
Parsing datafusion-ffi v55.1.0 (baseline)
Parsed [ 0.061s] (baseline)
Checking datafusion-ffi v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.308s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip
--- failure inherent_method_missing: pub method removed or renamed ---
Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/inherent_method_missing.ron
Failed in:
EmptyExec::with_partitioning, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/c719899b554402a07df4a8013c9a9bb859ade28d/datafusion/ffi/src/execution_plan.rs:603
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 108.955s] datafusion-ffi
Building datafusion-functions v55.1.0 (current)
Built [ 29.831s] (current)
Parsing datafusion-functions v55.1.0 (current)
Parsed [ 0.080s] (current)
Building datafusion-functions v55.1.0 (baseline)
Built [ 29.878s] (baseline)
Parsing datafusion-functions v55.1.0 (baseline)
Parsed [ 0.084s] (baseline)
Checking datafusion-functions v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.510s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 61.648s] datafusion-functions
Building datafusion-spark v55.1.0 (current)
Built [ 43.862s] (current)
Parsing datafusion-spark v55.1.0 (current)
Parsed [ 0.061s] (current)
Building datafusion-spark v55.1.0 (baseline)
Built [ 44.100s] (baseline)
Parsing datafusion-spark v55.1.0 (baseline)
Parsed [ 0.063s] (baseline)
Checking datafusion-spark v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.446s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 89.942s] datafusion-spark
Building datafusion-sql v55.1.0 (current)
Built [ 40.490s] (current)
Parsing datafusion-sql v55.1.0 (current)
Parsed [ 0.031s] (current)
Building datafusion-sql v55.1.0 (baseline)
Built [ 40.692s] (baseline)
Parsing datafusion-sql v55.1.0 (baseline)
Parsed [ 0.032s] (baseline)
Checking datafusion-sql v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.312s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 83.010s] datafusion-sql
Building datafusion-sqllogictest v55.1.0 (current)
Built [ 93.606s] (current)
Parsing datafusion-sqllogictest v55.1.0 (current)
Parsed [ 0.021s] (current)
Building datafusion-sqllogictest v55.1.0 (baseline)
Built [ 93.758s] (baseline)
Parsing datafusion-sqllogictest v55.1.0 (baseline)
Parsed [ 0.085s] (baseline)
Checking datafusion-sqllogictest v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.213s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 191.339s] datafusion-sqllogictest
```
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Validate `execution.time_zone` at `SET` time [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on PR #25788:
URL: https://github.com/apache/datafusion/pull/25788#issuecomment-5927272958
@KassaSana can you resolve the conflicts
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Validate `execution.time_zone` at `SET` time [datafusion]
Posted by "KassaSana (via GitHub)" <gi...@apache.org>.
KassaSana commented on PR #25788:
URL: https://github.com/apache/datafusion/pull/25788#issuecomment-5963218320
Thanks @kumarUjjawal, merged main and fixed the conflict in `from_unixtime.rs`. I also had to tweak a test that came in with #25668 (`scalar_function.rs`) since it was still setting `time_zone` as a plain `String`. Should be good now.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org