You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Weijun-H (via GitHub)" <gi...@apache.org> on 2025/10/11 09:46:47 UTC
[PR] fix: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Weijun-H opened a new pull request, #18017:
URL: https://github.com/apache/datafusion/pull/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 #123` indicates that this PR will close issue #123.
-->
- Closes #17993
## Rationale for this change
<!--
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?
<!--
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.
-->
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#issuecomment-3407847057
> This is looking good. I'd like to see an addition to the upgrade guide as this currently will be either a slight change in behaviour (timezone of None previously vs now it'll be Some("+00:00") with default config) or with a different tz set in the config it'll result in a different value.
>
> We may want to update the doc for ExecutionOptions::time_zone slightly as well as currently it's very targetted at 'Extract'.
>
> As a followup PR I think we could allow for when no tz is set in the config_options to use `None` vs `Some('')`.
tracked by #18081
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#issuecomment-3420950064
@kosiew @Jefffrey @Omega359 @alamb
Any additional feedback on this PR? If there are no blocking issues, I'd like to merge soon.
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#discussion_r2427099487
##########
datafusion/functions/src/datetime/now.rs:
##########
@@ -54,6 +57,15 @@ impl NowFunc {
Self {
signature: Signature::nullary(Volatility::Stable),
aliases: vec!["current_timestamp".to_string()],
+ timezone: Some(Arc::from("+00")),
Review Comment:
The hardcoded timezone '+00' should use a named constant to avoid magic values and ensure consistency across the codebase.
##########
datafusion/functions/src/datetime/now.rs:
##########
@@ -106,8 +118,16 @@ impl ScalarUDFImpl for NowFunc {
.execution_props()
.query_execution_start_time
.timestamp_nanos_opt();
+
+ let timezone = info
+ .execution_props()
+ .config_options
+ .as_ref()
+ .map(|opts| opts.execution.time_zone.as_str())
Review Comment:
The fallback timezone '+00' should use the same constant as used in the constructor to maintain consistency.
```suggestion
.map(|opts| opts.execution.time_zone.as_str())
.or(self.timezone.as_deref())
```
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#issuecomment-3404190108
Dropping a reminder here to uncomment relevant tests that were disabled by #18065 if this PR fixes some of them
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#discussion_r2427104195
##########
datafusion/functions/src/datetime/now.rs:
##########
@@ -80,7 +80,7 @@ impl ScalarUDFImpl for NowFunc {
fn return_field_from_args(&self, _args: ReturnFieldArgs) -> Result<FieldRef> {
Ok(Field::new(
self.name(),
- Timestamp(Nanosecond, Some("+00:00".into())),
+ Timestamp(Nanosecond, Some("+00".into())),
Review Comment:
It works now
```rust
> SET TIME ZONE = '+08';
0 row(s) fetched.
Elapsed 0.011 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.
> select now();
+----------------------------------+
| now() |
+----------------------------------+
| 2025-10-14T03:02:55.991869+08:00 |
+----------------------------------+
1 row(s) fetched.
Elapsed 0.006 seconds.
```
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#issuecomment-3431271375
Thanks again @kosiew @Jefffrey @Omega359 @alamb
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#discussion_r2432548305
##########
datafusion/functions/src/datetime/now.rs:
##########
@@ -54,6 +57,15 @@ impl NowFunc {
Self {
signature: Signature::nullary(Volatility::Stable),
aliases: vec!["current_timestamp".to_string()],
+ timezone: Some(Arc::from("+00")),
Review Comment:
Can someone explain the implications of this change on downstream crates? Does it mean that that `now()` will always uses the current timezone from the ConfigOptions?
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1072,6 +1072,28 @@ impl SessionContext {
} else {
let mut state = self.state.write();
state.config_mut().options_mut().set(&variable, &value)?;
+
+ // Re-initialize any UDFs that depend on configuration
+ // This allows both built-in and custom functions to respond to configuration changes
+ let config_options = state.config().options();
+ let udf_names: Vec<_> = state.scalar_functions().keys().cloned().collect();
Review Comment:
why do we need to "clone()" the keys (all the function names)? It seems like only the &name is used below
##########
datafusion/expr/src/udf.rs:
##########
@@ -532,6 +532,25 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
/// [`DataFusionError::Internal`]: datafusion_common::DataFusionError::Internal
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType>;
+ /// Create a new instance of this function with updated configuration.
+ ///
+ /// This method is called when configuration options change at runtime
+ /// (e.g., via `SET` statements) to allow functions that depend on
+ /// configuration to update themselves accordingly.
+ ///
+ /// # Arguments
+ ///
+ /// * `config` - The updated configuration options
+ ///
+ /// # Returns
+ ///
+ /// * `Some(ScalarUDF)` - A new instance of this function configured with the new settings
+ /// * `None` - If this function does not support runtime reconfiguration
Review Comment:
```suggestion
/// * `None` - If this function does not change with new configuration settings (the default)
```
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#discussion_r2431087451
##########
datafusion/functions/src/datetime/now.rs:
##########
@@ -54,6 +57,15 @@ impl NowFunc {
Self {
signature: Signature::nullary(Volatility::Stable),
aliases: vec!["current_timestamp".to_string()],
+ timezone: Some(Arc::from("+00")),
Review Comment:
NowFunc::new() (and therefore Default) now hardcodes the timezone as "+00".
Everywhere else in the codebase — including the default ConfigOptions and existing sqllogictests — expects the canonical "+00:00" form.
Won't this mean that any downstream caller that still constructs the UDF via NowFunc::new()/Default (for example, when registering their own function registry) now get a different, non-canonical offset that will no longer compare equal to the rest of the system?
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#discussion_r2433201019
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1073,6 +1075,18 @@ impl SessionContext {
let mut state = self.state.write();
state.config_mut().options_mut().set(&variable, &value)?;
drop(state);
+
+ // 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)?;
+ }
Review Comment:
I think the above approach will handle all future udf changes that follow this pattern
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#discussion_r2433190141
##########
datafusion/functions/src/datetime/now.rs:
##########
@@ -54,6 +57,15 @@ impl NowFunc {
Self {
signature: Signature::nullary(Volatility::Stable),
aliases: vec!["current_timestamp".to_string()],
+ timezone: Some(Arc::from("+00")),
Review Comment:
Yes, I agree
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#issuecomment-3407277942
This is looking good. I'd like to see an addition to the upgrade guide as this currently will be either a slight change in behaviour (timezone of None previously vs now it'll be Some("+00:00") with default config) or with a different tz set in the config it'll result in a different value.
We may want to update the doc for ExecutionOptions::time_zone slightly as well as currently it's very targetted at 'Extract'.
As a followup PR I think we could allow for when no tz is set in the config_options to use `None` vs `Some('')`.
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H merged PR #18017:
URL: https://github.com/apache/datafusion/pull/18017
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#issuecomment-3432244001
> LGTM except for the deprecated NowFunc::new(), still seeding the timezone with "+00"
Filed as https://github.com/apache/datafusion/issues/18219
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#discussion_r2429706593
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1072,6 +1074,16 @@ impl SessionContext {
} else {
let mut state = self.state.write();
state.config_mut().options_mut().set(&variable, &value)?;
+
+ // 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" {
Review Comment:
Hardcoding the variable and the function here I don't think is the correct approach. It may work for the few functions provided by DF that require this but it won't help any custom functions that might require this ability.
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#discussion_r2429760157
##########
datafusion/expr/src/udf.rs:
##########
@@ -532,6 +536,11 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
/// [`DataFusionError::Internal`]: datafusion_common::DataFusionError::Internal
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType>;
+
+ fn need_config(&self) -> bool {
Review Comment:
I believe this would be better without &self so that it could be called as an associated function rather than a method on an instance.
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#issuecomment-3400913828
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.
```
❯ DATAFUSION_EXECUTION_TIME_ZONE='+05:45' cargo run --bin datafusion-cli -- -c "SELECT arrow_typeof(now());"
DataFusion CLI v50.1.0
+------------------------------------+
| arrow_typeof(now()) |
+------------------------------------+
| Timestamp(Nanosecond, Some("+00")) |
+------------------------------------+
1 row(s) fetched.
Elapsed 0.092 seconds.
```
```
❯ cargo run --bin datafusion-cli -- -c "SET datafusion.execution.time_zone = '+05:45'; SELECT arrow_typeof(now());"
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.48s
Running `target/debug/datafusion-cli -c 'SET datafusion.execution.time_zone = '\''+05:45'\''; SELECT arrow_typeof(now());'`
DataFusion CLI v50.1.0
0 row(s) fetched.
Elapsed 0.034 seconds.
+---------------------------------------+
| arrow_typeof(now()) |
+---------------------------------------+
| Timestamp(Nanosecond, Some("+05:45")) |
+---------------------------------------+
1 row(s) fetched.
Elapsed 0.040 seconds.
```
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#issuecomment-3403434100
Thanks @Omega359 and @kosiew for reviewing. I think this pr is ready for review.
```
╰─ DATAFUSION_EXECUTION_TIME_ZONE='+05:45' cargo run --bin datafusion-cli -- -c "SELECT arrow_typeof(now());" ─╯
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.53s
Running `target/debug/datafusion-cli -c 'SELECT arrow_typeof(now());'`
DataFusion CLI v50.1.0
+---------------------------------------+
| arrow_typeof(now()) |
+---------------------------------------+
| Timestamp(Nanosecond, Some("+05:45")) |
+---------------------------------------+
1 row(s) fetched.
Elapsed 0.011 seconds.
```
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#discussion_r2433180096
##########
datafusion/functions/src/datetime/now.rs:
##########
@@ -54,6 +57,15 @@ impl NowFunc {
Self {
signature: Signature::nullary(Volatility::Stable),
aliases: vec!["current_timestamp".to_string()],
+ timezone: Some(Arc::from("+00")),
Review Comment:
Update: Yes, this is the implication
##########
datafusion/expr/src/udf.rs:
##########
@@ -532,6 +532,25 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
/// [`DataFusionError::Internal`]: datafusion_common::DataFusionError::Internal
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType>;
+ /// Create a new instance of this function with updated configuration.
+ ///
+ /// This method is called when configuration options change at runtime
+ /// (e.g., via `SET` statements) to allow functions that depend on
+ /// configuration to update themselves accordingly.
Review Comment:
I think it would be useful to document one of the usecases, namely that it should be used when the type of the expression depends on the configuration values.
For example, something like this:
```suggestion
/// This method is called when configuration options change at runtime
/// (e.g., via `SET` statements) to allow functions that depend on
/// configuration to update themselves accordingly.
///
/// Note the current [`ConfigOptions`] are also passed to [`Self::invoke_with_args`] so
/// this API is not needed for functions where the values may
/// depend on the current options.
///
/// This API is useful for functions where the return
/// **type** depends on the configuration options, such as the `now()` function
/// which depends on the current timezone.
```
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#discussion_r2422963167
##########
datafusion/functions/src/datetime/now.rs:
##########
@@ -80,7 +80,7 @@ impl ScalarUDFImpl for NowFunc {
fn return_field_from_args(&self, _args: ReturnFieldArgs) -> Result<FieldRef> {
Ok(Field::new(
self.name(),
- Timestamp(Nanosecond, Some("+00:00".into())),
+ Timestamp(Nanosecond, Some("+00".into())),
Review Comment:
This unfortunately won't work. If you change the execution time to say +08 and try and run the query in [this comment](https://github.com/apache/datafusion/issues/17993#issuecomment-3386013811) you'll see it fails.
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#discussion_r2427108577
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1073,6 +1075,18 @@ impl SessionContext {
let mut state = self.state.write();
state.config_mut().options_mut().set(&variable, &value)?;
drop(state);
+
+ // 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)?;
+ }
Review Comment:
We could refactor this logic after these issue resolved:
- https://github.com/apache/datafusion/issues/17995
- https://github.com/apache/datafusion/issues/17998
- https://github.com/apache/datafusion/issues/17995
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#discussion_r2431557975
##########
datafusion/functions/src/datetime/now.rs:
##########
@@ -54,6 +57,15 @@ impl NowFunc {
Self {
signature: Signature::nullary(Volatility::Stable),
aliases: vec!["current_timestamp".to_string()],
+ timezone: Some(Arc::from("+00")),
Review Comment:
I think we could remove `new()`, because `now` relies on config for the initialization
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#discussion_r2433182045
##########
datafusion/functions/src/datetime/now.rs:
##########
@@ -54,6 +57,15 @@ impl NowFunc {
Self {
signature: Signature::nullary(Volatility::Stable),
aliases: vec!["current_timestamp".to_string()],
+ timezone: Some(Arc::from("+00")),
Review Comment:
@alamb With this implementation, yes. To me the logical solution is to allow the time_zone config option to be an Option (and default to None) and fix any existing usage to handle that ... or to detect when the time_zone config option is '' and handle that in the same manner ('' -> None tz)
--
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: Use dynamic timezone in now() function for accurate timestamp [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #18017:
URL: https://github.com/apache/datafusion/pull/18017#discussion_r2444922735
##########
datafusion/functions/src/datetime/now.rs:
##########
@@ -54,6 +57,15 @@ impl NowFunc {
Self {
signature: Signature::nullary(Volatility::Stable),
aliases: vec!["current_timestamp".to_string()],
+ timezone: Some(Arc::from("+00")),
Review Comment:
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.
--
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