-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: Use dynamic timezone in now() function for accurate timestamp #18017
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Use dynamic timezone in now() function for accurate timestamp #18017
Conversation
8db266f to
33732c5
Compare
70ed548 to
b81ebf6
Compare
f7ba4e5 to
9f1b55f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the now() function to return timestamps with the correct timezone set in the session configuration, instead of always using the default "+00:00" timezone.
Key changes:
- Updated
now()function to use dynamic timezone from session configuration - Added configuration-aware UDF creation macro for functions that depend on session state
- Added test coverage for timezone-aware
now()function behavior
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| datafusion/functions/src/macros.rs | Added new macro for creating UDFs with configuration parameters |
| datafusion/functions/src/datetime/now.rs | Modified NowFunc to accept and use timezone from configuration instead of hardcoded "+00:00" |
| datafusion/core/src/execution/context/mod.rs | Added logic to re-register now() UDF when timezone configuration changes |
| datafusion/sqllogictest/test_files/timestamps.slt | Added test to verify now() returns correct timezone type |
| datafusion/sqllogictest/test_files/dates.slt | Updated expected error message to reflect new default timezone format |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| // Register UDFs that return values based on session configuration | ||
| // e.g. now() which depends on the time_zone configuration option | ||
| if variable == "datafusion.execution.time_zone" { | ||
| let config_options = self.state.read().config().options().clone(); | ||
| let now_udf = { | ||
| // recreate the function so it captures the new time zone | ||
| make_udf_function_with_config!(NowFunc, now, &ConfigOptions); | ||
| now(&config_options) | ||
| }; | ||
| self.state.write().register_udf(now_udf)?; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could refactor this logic after these issue resolved:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the above approach will handle all future udf changes that follow this pattern
34ad88f to
e7a6ab9
Compare
8a1de2c to
ed73c86
Compare
|
Re-registering now() inside set_config_option covers interactive SET commands, but sessions that are constructed differently eg with a pre-configured SessionConfig (e.g. via builder APIs or server defaults) still get the default NowFunc::new() that ignores the configured timezone. eg. |
|
Thanks @Omega359 and @kosiew for reviewing. I think this pr is ready for review. |
1310d0f to
179607a
Compare
Co-authored-by: Andrew Lamb <[email protected]>
f9fbf67 to
4e8a0c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for the deprecated NowFunc::new(), still seeding the timezone with "+00"
| Self { | ||
| signature: Signature::nullary(Volatility::Stable), | ||
| aliases: vec!["current_timestamp".to_string()], | ||
| timezone: Some(Arc::from("+00")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NowFunc::new(), though deprecated seeds the timezone with "+00" (missing minutes) while the canonical UTC offset remains "+00:00" in ConfigOptions::default().
Existing callers of the deprecated constructor will therefore get a different datatype/ScalarValue than before this change.
Filed as #18219 |
## Which issue does this PR close? - Relates #18062 - Relates #18065 ## Rationale for this change We disabled these tests because CI was failing on main. The test: `current_date() = cast(now() as date)` was added in #18034 requires `now` to use configured timezone, but it is only available after #18017. Since #18017 has been merged, these tests should be enable. ## What changes are included in this PR? This reverts commit a65a2cb. ## Are these changes tested? Yes. ## Are there any user-facing changes? No.
…pache#18017) ## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes apache#17993 ## Rationale for this change ``` DataFusion CLI v50.1.0 > SET TIME ZONE = '+08:00'; 0 row(s) fetched. Elapsed 0.011 seconds. > SELECT arrow_typeof(now()); +---------------------------------------+ | arrow_typeof(now()) | +---------------------------------------+ | Timestamp(Nanosecond, Some("+08:00")) | +---------------------------------------+ 1 row(s) fetched. Elapsed 0.015 seconds. > SELECT count(1) result FROM (SELECT now() as n) a WHERE n > '2000-01-01'::date; +--------+ | result | +--------+ | 1 | +--------+ 1 row(s) fetched. Elapsed 0.029 seconds. ``` <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> ## What changes are included in this PR? When the timezone changes, re-register `now()` function <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> ## Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. --> --------- Co-authored-by: Andrew Lamb <[email protected]>
## Which issue does this PR close? - Relates apache#18062 - Relates apache#18065 ## Rationale for this change We disabled these tests because CI was failing on main. The test: `current_date() = cast(now() as date)` was added in apache#18034 requires `now` to use configured timezone, but it is only available after apache#18017. Since apache#18017 has been merged, these tests should be enable. ## What changes are included in this PR? This reverts commit a65a2cb. ## Are these changes tested? Yes. ## Are there any user-facing changes? No.
Which issue does this PR close?
nowaware of execution timezone #17993Rationale for this change
What changes are included in this PR?
When the timezone changes, re-register
now()functionAre these changes tested?
Are there any user-facing changes?