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: [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]
