support `SET Timezone` #4107

waitingkuo · 2022-11-04T11:38:59Z

Which issue does this PR close?

Rationale for this change

a POC for #4106

to support set time zone

❯ show time zone;
+--------------------------------+---------+
| name                           | setting |
+--------------------------------+---------+
| datafusion.execution.time_zone | UTC     |
+--------------------------------+---------+
1 row in set. Query took 0.069 seconds.

❯ set time zone = '+08:00';
0 rows in set. Query took 0.001 seconds.

❯ show time zone;
+--------------------------------+---------+
| name                           | setting |
+--------------------------------+---------+
| datafusion.execution.time_zone | +08:00  |
+--------------------------------+---------+
1 row in set. Query took 0.007 seconds.

casting to timestamptz should use the timezone instead of the fixed UTC

❯ select arrow_typeof('2000-01-01T00:00:00'::timestamp::timestamptz);
+------------------------------------------+
| arrowtypeof(Utf8("2000-01-01T00:00:00")) |
+------------------------------------------+
| Timestamp(Nanosecond, Some("+08:00"))    |
+------------------------------------------+
1 row in set. Query took 0.007 seconds.

What changes are included in this PR?

Are there any user-facing changes?

waitingkuo · 2022-11-04T11:43:27Z

datafusion/sql/src/planner.rs

+    fn convert_simple_data_type(&self, sql_type: &SQLDataType) -> Result<DataType> {
+        match sql_type {
+            SQLDataType::Boolean => Ok(DataType::Boolean),
+            SQLDataType::TinyInt(_) => Ok(DataType::Int8),
+            SQLDataType::SmallInt(_) => Ok(DataType::Int16),
+            SQLDataType::Int(_) | SQLDataType::Integer(_) => Ok(DataType::Int32),
+            SQLDataType::BigInt(_) => Ok(DataType::Int64),
+            SQLDataType::UnsignedTinyInt(_) => Ok(DataType::UInt8),
+            SQLDataType::UnsignedSmallInt(_) => Ok(DataType::UInt16),


becomes method of SqlToRel, so that we could get SessionState -> config_options -> timezone

waitingkuo · 2022-11-04T11:46:11Z

datafusion/sql/src/planner.rs

+            SQLDataType::Timestamp(tz_info) => {
+                let tz = if matches!(tz_info, TimezoneInfo::Tz)
+                    || matches!(tz_info, TimezoneInfo::WithTimeZone)
+                {
+                    match self.schema_provider.get_config_option("datafusion.execution.time_zone") {
+                        Some(ScalarValue::Utf8(s)) => {
+                            s
+                        }                            
+                        Some(_) => {
+                            None
+                        }
+                        None => None
+                    }
+                    //Some("+00:00".to_string())
+
+                } else {
+                    None
+                };
+                Ok(DataType::Timestamp(TimeUnit::Nanosecond, tz))


as this becomes the method of SqltoRel, we can now get the timezone and use it to replace the fixed UTC
note that we need to add some timezone validation here (e.g. 00:00.1 isn't valid)

It is somewhat strange to me that the type of a table that is created depends on the current session 's timezone setting -- but I can't think of anything better short of the user explicitly providing the timezone themselves

Sql's TimestampTz determine the timezone during the execution (perhaps right before it's been execute). arrow doesn't support TimestampTz in nature. what arrow has here is Timestamp<TimeUnit, Some(tz)>.

I've been thinking about how to deal with this for a long time 🤮 . I have 2 approaches to do so

add some thing like Timestamp<TimeUnit, Some("session")> All the time related functions/kernels need to convert this "session" to the timezone by themselves. i.e. fn week() accept Timestamp<TimeUnit, Some("session")> as input and use the timezone in config_options to convert the underline timestamp and then do the week extraction. This will be more that the TImestampTz in SQL

arrow's Timestamp<TImeUnit, Some("some-timezone")> is determined while SQL is translated to arrow's data type. and this is what this pr is using.

SQL's TimestampTz isn't deterministic while making arrow's data deterministic makes more sense to me; so i picked the 2nd approach for now. But i'm happy to try the first approach if anyone is interested in

I agree the second approach makes sense to me.

Maybe usability wise we can just add enough comments about what is happening, or maybe add that best practice is to explicitly specify the timezone in the create table statement / timestamp types.

waitingkuo · 2022-11-04T11:47:41Z

datafusion/sql/src/planner.rs

+/*
 pub fn convert_simple_data_type(sql_type: &SQLDataType) -> Result<DataType> {
    match sql_type {


this is the original function which is now the method of SqlToRel

perhaps we should a deprecated notifunction here as this is a pub function

I think we should remove it personally or of we want to deprecate it for a release cycle that would be fine

Bonus points for somehow calling the new implementation (like making a dummy SqlToRel somehow)

alamb

I think this is looking great !

alamb · 2022-11-04T16:33:49Z

datafusion/core/src/execution/context.rs

@@ -1741,6 +1741,12 @@ impl ContextProvider for SessionState {
            .as_ref()
            .and_then(|provider| provider.get(&provider_type)?.get_type(variable_names))
    }
+
+    fn get_config_option(&self, variable: &str) -> Option<ScalarValue> {


alamb · 2022-11-04T16:40:36Z

datafusion/sql/src/planner.rs

+            SQLDataType::Timestamp(tz_info) => {
+                let tz = if matches!(tz_info, TimezoneInfo::Tz)
+                    || matches!(tz_info, TimezoneInfo::WithTimeZone)
+                {
+                    match self.schema_provider.get_config_option("datafusion.execution.time_zone") {
+                        Some(ScalarValue::Utf8(s)) => {
+                            s
+                        }                            
+                        Some(_) => {
+                            None
+                        }
+                        None => None
+                    }
+                    //Some("+00:00".to_string())
+
+                } else {
+                    None
+                };
+                Ok(DataType::Timestamp(TimeUnit::Nanosecond, tz))


It is somewhat strange to me that the type of a table that is created depends on the current session 's timezone setting -- but I can't think of anything better short of the user explicitly providing the timezone themselves

alamb · 2022-11-04T16:41:07Z

datafusion/sql/src/planner.rs

+                let tz = if matches!(tz_info, TimezoneInfo::Tz)
+                    || matches!(tz_info, TimezoneInfo::WithTimeZone)
+                {
+                    match self.schema_provider.get_config_option("datafusion.execution.time_zone") {


It would be nice to use a named constant instead of the string here

@alamb named constants are in datafusion/core. i'm not sure how to import them from here (due to circular import)

🤔 maybe we should eventually move the config option stuff to common? I have a todo on my list to write up a ticket about rationalizing the config system - if no one else beats me to it I'll probably start in a week or two

alamb · 2022-11-04T16:44:45Z

datafusion/sql/src/planner.rs

+/*
 pub fn convert_simple_data_type(sql_type: &SQLDataType) -> Result<DataType> {
    match sql_type {


I think we should remove it personally or of we want to deprecate it for a release cycle that would be fine

Bonus points for somehow calling the new implementation (like making a dummy SqlToRel somehow)

waitingkuo

@alamb @avantgardnerio
i made some updates

fix examples
limit the usage to fixedoffset (like +08:00)
add test cases for different timezone format
delete pub fn convert_simple_data_type, it becomes the method of SqlToRel
still use dataufion.execution.time_zone instead of TIME_ZONE_OPT since importing constant variable in sql/planners introduces circular import.

waitingkuo · 2022-11-07T10:20:21Z

datafusion/sql/src/planner.rs

-/// Convert SQL simple data type to relational representation of data type
-pub fn convert_simple_data_type(sql_type: &SQLDataType) -> Result<DataType> {
-    match sql_type {
-        SQLDataType::Boolean => Ok(DataType::Boolean),
-        SQLDataType::TinyInt(_) => Ok(DataType::Int8),
-        SQLDataType::SmallInt(_) => Ok(DataType::Int16),
-        SQLDataType::Int(_) | SQLDataType::Integer(_) => Ok(DataType::Int32),
-        SQLDataType::BigInt(_) => Ok(DataType::Int64),
-        SQLDataType::UnsignedTinyInt(_) => Ok(DataType::UInt8),
-        SQLDataType::UnsignedSmallInt(_) => Ok(DataType::UInt16),
-        SQLDataType::UnsignedInt(_) | SQLDataType::UnsignedInteger(_) => {
-            Ok(DataType::UInt32)
-        }


@alamb i deleted it

I marked the PR as "API change"

waitingkuo · 2022-11-07T10:23:14Z

datafusion/sql/src/planner.rs

+                    match self
+                        .schema_provider
+                        .get_config_option("datafusion.execution.time_zone")
+                    {
+                        Some(ScalarValue::Utf8(s)) => s,
+                        Some(v) => {
+                            return Err(DataFusionError::Internal(format!(
+                                "Incorrect data type for time_zone: {}",
+                                v.get_datatype(),
+                            )))
+                        }
+                        None => return Err(DataFusionError::Internal(format!(
+                            "Config Option datafusion.execution.time_zone doesn't exist"
+                        ))),


ScalarValue::UTF8 -> Option<String>
other ScalarValue -> Err
None (variable name doen't exsit) -> Err

waitingkuo · 2022-11-07T10:25:55Z