Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions datafusion/core/src/execution/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ use datafusion_expr::{
planner::ExprPlanner,
Expr, UserDefinedLogicalNode, WindowUDF,
};
use datafusion_functions::datetime::now::NowFunc;
use datafusion_functions::make_udf_function_with_config;
use datafusion_optimizer::analyzer::type_coercion::TypeCoercion;
use datafusion_optimizer::Analyzer;
use datafusion_optimizer::{AnalyzerRule, OptimizerRule};
Expand Down Expand Up @@ -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)?;
}
Comment on lines 1075 to 1093
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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

}

self.return_empty_dataframe()
Expand Down
24 changes: 22 additions & 2 deletions datafusion/functions/src/datetime/now.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ use arrow::datatypes::DataType::Timestamp;
use arrow::datatypes::TimeUnit::Nanosecond;
use arrow::datatypes::{DataType, Field, FieldRef};
use std::any::Any;
use std::sync::Arc;

use datafusion_common::config::ConfigOptions;
use datafusion_common::{internal_err, Result, ScalarValue};
use datafusion_expr::simplify::{ExprSimplifyResult, SimplifyInfo};
use datafusion_expr::{
Expand All @@ -41,6 +43,7 @@ The `now()` return value is determined at query time and will return the same ti
pub struct NowFunc {
signature: Signature,
aliases: Vec<String>,
timezone: Option<Arc<str>>,
}

impl Default for NowFunc {
Expand All @@ -54,6 +57,15 @@ impl NowFunc {
Self {
signature: Signature::nullary(Volatility::Stable),
aliases: vec!["current_timestamp".to_string()],
timezone: Some(Arc::from("+00")),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could remove new(), because now relies on config for the initialization

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: Yes, this is the implication

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree

}
}

pub fn new_with_config(config: &ConfigOptions) -> Self {
Self {
signature: Signature::nullary(Volatility::Stable),
aliases: vec!["current_timestamp".to_string()],
timezone: Some(Arc::from(config.execution.time_zone.as_str())),
}
}
}
Expand All @@ -80,7 +92,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, self.timezone.clone()),
false,
)
.into())
Expand All @@ -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())
.unwrap_or("+00");

Ok(ExprSimplifyResult::Simplified(Expr::Literal(
ScalarValue::TimestampNanosecond(now_ts, Some("+00:00".into())),
ScalarValue::TimestampNanosecond(now_ts, Some(timezone.into())),
None,
)))
}
Expand Down
16 changes: 16 additions & 0 deletions datafusion/functions/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,22 @@ macro_rules! make_udf_function {
};
}

/// Creates a singleton `ScalarUDF` of the `$UDF` function and a function
/// named `$NAME` which returns that singleton. The function takes a
/// configuration argument of type `$CONFIG_TYPE` to create the UDF.
#[macro_export]
macro_rules! make_udf_function_with_config {
($UDF:ty, $NAME:ident, $CONFIG_TYPE:ty) => {
#[allow(rustdoc::redundant_explicit_links)]
#[doc = concat!("Return a [`ScalarUDF`](datafusion_expr::ScalarUDF) implementation of ", stringify!($NAME))]
pub fn $NAME(config: $CONFIG_TYPE) -> std::sync::Arc<datafusion_expr::ScalarUDF> {
std::sync::Arc::new(datafusion_expr::ScalarUDF::new_from_impl(
<$UDF>::new_with_config(&config),
))
}
};
}

/// Macro creates a sub module if the feature is not enabled
///
/// The rationale for providing stub functions is to help users to configure datafusion
Expand Down
2 changes: 1 addition & 1 deletion datafusion/sqllogictest/test_files/dates.slt
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ g
h

## Plan error when compare Utf8 and timestamp in where clause
statement error DataFusion error: type_coercion\ncaused by\nError during planning: Cannot coerce arithmetic expression Timestamp\(Nanosecond, Some\("\+00:00"\)\) \+ Utf8 to valid types
statement error DataFusion error: type_coercion\ncaused by\nError during planning: Cannot coerce arithmetic expression Timestamp\(Nanosecond, Some\("\+00"\)\) \+ Utf8 to valid types
select i_item_desc from test
where d3_date > now() + '5 days';

Expand Down
10 changes: 10 additions & 0 deletions datafusion/sqllogictest/test_files/timestamps.slt
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,16 @@ true
##########
## Current time Tests
##########
statement ok
SET TIME ZONE = '+08'

query T
select arrow_typeof(now());
----
Timestamp(Nanosecond, Some("+08"))

statement ok
SET TIME ZONE = '+00'

query B
select cast(now() as time) = current_time();
Expand Down
Loading