fix: validate time zone config before applying changes - #23224
fix: validate time zone config before applying changes#23224Probablism wants to merge 3 commits into
Conversation
|
would it be possible to enforce it more strictly by changing the type of the config to datafusion/datafusion/common/src/config.rs Lines 770 to 773 in ea2ffdb |
5ab1228 to
dbd8d10
Compare
dbd8d10 to
5ab1228
Compare
5ab1228 to
758a3a9
Compare
|
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 |
Jefffrey
left a comment
There was a problem hiding this comment.
for the most part this looks good, just wondering if there'll be downstream impacts to warrant marking this a breaking API change 🤔
| config.options_mut().execution.time_zone = Some("AEST".into()); | ||
| config.options_mut().execution.time_zone = Some("Australia/Sydney".parse().unwrap()); |
There was a problem hiding this comment.
this change is interesting; were the tests just flawed previously and AEST was never a valid timezone (but we never tried parsing it)?
| select arrow_typeof(now()); | ||
| ---- | ||
| Timestamp(ns, "+08") | ||
| Timestamp(ns, "+08:00") |
There was a problem hiding this comment.
hm i wonder if this is significant
i wonder if we have some precedent from other engines (duckdb, spark) if they handle in a similar way or not
Which issue does this PR close?
Rationale for this change
datafusion.execution.time_zoneaccepted a raw string value, so invalidtimezones 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?
ConfigTimeZonewrapper around Arrow's parsed timezone type.ExecutionOptions::time_zonefromOption<String>toOption<ConfigTimeZone>.value and only convert back to a string where Arrow metadata/display requires
it.
SQL
SETbehavior, and UDF/FFI config propagation.Are these changes tested?
Yes.
cargo fmt --allcargo clippy --all-targets --all-features -- -D warningscargo test -p datafusion-common test_execution_time_zone_validationcargo test -p datafusion --test user_defined_integration test_config_options_work_for_scalar_funccargo test -p datafusion-ffi --features integration-tests --test ffi_udf test_config_on_scalar_udfcargo test -p datafusion-functions timezonecargo test --profile=ci --test sqllogictests -- set_variablecargo check -p datafusion-common -p datafusion-sql -p datafusion-functions -p datafusion-spark -p datafusion-ffi -p datafusion --testsAre there any user-facing changes?
Yes. Invalid
SET TIME ZONEvalues now fail immediately instead of beingaccepted and causing later timestamp operations to fail.