Skip to content

fix: validate time zone config before applying changes#23224

Open
Probablism wants to merge 2 commits into
apache:mainfrom
Probablism:issue-17498-timezone-validation
Open

fix: validate time zone config before applying changes#23224
Probablism wants to merge 2 commits into
apache:mainfrom
Probablism:issue-17498-timezone-validation

Conversation

@Probablism

@Probablism Probablism commented Jun 28, 2026

Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

datafusion.execution.time_zone accepted a raw string value, so invalid
timezones could be stored in the session config and fail later when timestamp
operations tried to parse them.

This changes the config value to store a parsed timezone, so invalid timezone
values are rejected when the setting is applied and the previous valid value
remains active.

What changes are included in this PR?

  • Add a ConfigTimeZone wrapper around Arrow's parsed timezone type.
  • Change ExecutionOptions::time_zone from Option<String> to
    Option<ConfigTimeZone>.
  • Update datetime, SQL, Spark, UDF, and FFI call sites to use the typed config
    value and only convert back to a string where Arrow metadata/display requires
    it.
  • Add regression coverage for valid values, invalid values, nested config keys,
    SQL SET behavior, and UDF/FFI config propagation.

Are these changes tested?

Yes.

  • cargo fmt --all
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo test -p datafusion-common test_execution_time_zone_validation
  • cargo test -p datafusion --test user_defined_integration test_config_options_work_for_scalar_func
  • cargo test -p datafusion-ffi --features integration-tests --test ffi_udf test_config_on_scalar_udf
  • cargo test -p datafusion-functions timezone
  • cargo test --profile=ci --test sqllogictests -- set_variable
  • cargo check -p datafusion-common -p datafusion-sql -p datafusion-functions -p datafusion-spark -p datafusion-ffi -p datafusion --tests

Are there any user-facing changes?

Yes. Invalid SET TIME ZONE values now fail immediately instead of being
accepted and causing later timestamp operations to fail.

@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) common Related to common crate labels Jun 28, 2026
@Jefffrey

Copy link
Copy Markdown
Contributor

would it be possible to enforce it more strictly by changing the type of the config to Tz? or is this infeasible for other reasons

/// The default time zone
///
/// Some functions, e.g. `now` return timestamps in this time zone
pub time_zone: Option<String>, default = None

@Probablism Probablism force-pushed the issue-17498-timezone-validation branch from 5ab1228 to dbd8d10 Compare June 28, 2026 16:23
@github-actions github-actions Bot added sql SQL Planner core Core DataFusion crate functions Changes to functions implementation ffi Changes to the ffi crate spark labels Jun 28, 2026
@Probablism Probablism force-pushed the issue-17498-timezone-validation branch from dbd8d10 to 5ab1228 Compare June 28, 2026 16:30
@Probablism Probablism force-pushed the issue-17498-timezone-validation branch from 5ab1228 to 758a3a9 Compare June 28, 2026 18:08
@Probablism

Copy link
Copy Markdown
Author

Good point. I had kept this as a string initially to avoid changing the config type, but I agree the typed version is better here.

I updated it to store a parsed timezone via ConfigTimeZone. It wraps Arrow's Tz because the config structs need PartialEq, and Tz doesn't provide that directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common Related to common crate core Core DataFusion crate ffi Changes to the ffi crate functions Changes to functions implementation spark sql SQL Planner sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants