add with_timezone #5906

waitingkuo · 2023-04-07T07:01:40Z

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

a new function with_timezone that replace the timezone of timestamp without changing underline value

❯ select WITH_TIMEZONE(timestamp '2000-01-01T00:00:00.123456789', '+08:00');
+--------------------------------------------------------------------+
| withtimezone(Utf8("2000-01-01T00:00:00.123456789"),Utf8("+08:00")) |
+--------------------------------------------------------------------+
| 2000-01-01T08:00:00.123456789+08:00                                |
+--------------------------------------------------------------------+
1 row in set. Query took 0.004 seconds.

❯ select WITH_TIMEZONE(WITH_TIMEZONE(timestamp '2000-01-01T00:00:00.123456789', '+08:00'), '+07:00');
+-------------------------------------------------------------------------------------------------+
| withtimezone(withtimezone(Utf8("2000-01-01T00:00:00.123456789"),Utf8("+08:00")),Utf8("+07:00")) |
+-------------------------------------------------------------------------------------------------+
| 2000-01-01T07:00:00.123456789+07:00                                                             |
+-------------------------------------------------------------------------------------------------+
1 row in set. Query took 0.003 seconds.

while the timezone is incorrect, it raises error

❯ select WITH_TIMEZONE(timestamp '2000-01-01T00:00:00.123456789', '+25:00');
Arrow error: Parser error: Invalid timezone "+25:00": '+25:00' is not a valid timezone

Are these changes tested?

yes, test cases are added to timestamps.slt

Are there any user-facing changes?

waitingkuo

since there's no way to extract timezonefn return_type , the return data_type is corrected in create_physical_expr
@alamb please help to evaluate whether it make sense

cc @tustvold

waitingkuo · 2023-04-07T07:10:09Z

datafusion/expr/src/function.rs

+        BuiltinScalarFunction::WithTimezone => match input_expr_types[0] {
+            DataType::Timestamp(TimeUnit::Nanosecond, _) => {
+                Ok(DataType::Timestamp(TimeUnit::Nanosecond, None))
+            }
+            DataType::Timestamp(TimeUnit::Microsecond, _) => {
+                Ok(DataType::Timestamp(TimeUnit::Microsecond, None))
+            }
+            DataType::Timestamp(TimeUnit::Millisecond, _) => {
+                Ok(DataType::Timestamp(TimeUnit::Millisecond, None))
+            }
+            DataType::Timestamp(TimeUnit::Second, _) => {
+                Ok(DataType::Timestamp(TimeUnit::Second, None))
+            }
+            _ => return Err(DataFusionError::Internal(
+                "The with_timezone function can only accept timestamp as the first arg."
+                    .to_string(),
+            )),
+        },


pub fn return_type( fun: &BuiltinScalarFunction, input_expr_types: &[DataType], ) -> Result<DataType> {

this function only has 2 inputs: function pointer and args types. there's no way to inference timezone in the final return type

I think you may need to special case it, in the same way as arrow_cast: https://github.com/apache/arrow-datafusion/blob/f00ef9d5484039dc7013af5b8fa04f7b29ffaba7/datafusion/sql/src/expr/function.rs#L139-L142

waitingkuo · 2023-04-07T07:16:26Z

datafusion/physical-expr/src/functions.rs

+            let timezone = format!("{}", input_phy_exprs[1]);
+            data_type = match data_type {
+                DataType::Timestamp(TimeUnit::Nanosecond, _) => DataType::Timestamp(TimeUnit::Nanosecond, Some(timezone)),
+                DataType::Timestamp(TimeUnit::Microsecond, _) => DataType::Timestamp(TimeUnit::Microsecond, Some(timezone)),
+                DataType::Timestamp(TimeUnit::Millisecond, _) => DataType::Timestamp(TimeUnit::Millisecond, Some(timezone)),
+                DataType::Timestamp(TimeUnit::Second, _) => DataType::Timestamp(TimeUnit::Second, Some(timezone)),
+                other => return Err(DataFusionError::Internal(format!(
+                    "Unsupported data type {other:?} as the first arg for function with_timezone",
+                )))
+            };


correct the timezone in return data_type according to the args value

@alamb is this implementation ok for now? or it's recommended to modify the return_type function to accept some new args to achieve this?

I could submit another ticket to modify it as well

alamb

Thanks @waitingkuo --

Even though arrow_cast could (and probably should) be updated to support timezones having a specific WITH_TIMEZONE seems like a good idea to me as it will be a better experience

I think this PR needs

Get the type correct (I left a comment about how to do this)
Add an entry to WITH_TIMEZONE the user documentation (e.g. https://github.com/apache/arrow-datafusion/blob/f00ef9d5484039dc7013af5b8fa04f7b29ffaba7/docs/source/user-guide/sql/scalar_functions.md?plain=1#L1208)

alamb · 2023-04-07T10:58:12Z

datafusion/expr/src/function.rs

+        BuiltinScalarFunction::WithTimezone => match input_expr_types[0] {
+            DataType::Timestamp(TimeUnit::Nanosecond, _) => {
+                Ok(DataType::Timestamp(TimeUnit::Nanosecond, None))
+            }
+            DataType::Timestamp(TimeUnit::Microsecond, _) => {
+                Ok(DataType::Timestamp(TimeUnit::Microsecond, None))
+            }
+            DataType::Timestamp(TimeUnit::Millisecond, _) => {
+                Ok(DataType::Timestamp(TimeUnit::Millisecond, None))
+            }
+            DataType::Timestamp(TimeUnit::Second, _) => {
+                Ok(DataType::Timestamp(TimeUnit::Second, None))
+            }
+            _ => return Err(DataFusionError::Internal(
+                "The with_timezone function can only accept timestamp as the first arg."
+                    .to_string(),
+            )),
+        },


I think you may need to special case it, in the same way as arrow_cast: https://github.com/apache/arrow-datafusion/blob/f00ef9d5484039dc7013af5b8fa04f7b29ffaba7/datafusion/sql/src/expr/function.rs#L139-L142

waitingkuo · 2023-04-07T11:38:10Z

Thanks @waitingkuo --

Even though arrow_cast could (and probably should) be updated to support timezones having a specific WITH_TIMEZONE seems like a good idea to me as it will be a better experience

I think this PR needs

Get the type correct (I left a comment about how to do this)

Add an entry to WITH_TIMEZONE the user documentation (e.g. https://github.com/apache/arrow-datafusion/blob/f00ef9d5484039dc7013af5b8fa04f7b29ffaba7/docs/source/user-guide/sql/scalar_functions.md?plain=1#L1208
)

Hi @alamb thanks, in fact i'm working on adding timezone support for arrow_cast now, will send a pr soon

tustvold · 2023-04-07T11:40:31Z

In fact i'm working on adding timezone support for arrow_cast now, will send a pr soon

I think we may want to fix apache/arrow-rs#1936 upstream, before exposing the currently incorrect timezone casting behaviour in arrow-rs?

TBC this is not a comment on this PR, the metadata only with_timezone added by this PR is good imo, but this should not be the behaviour of arrow-cast, which should instead preserve the same time "instant" as per the arrow specification

waitingkuo · 2023-04-07T14:06:29Z

Thanks @waitingkuo --

Even though arrow_cast could (and probably should) be updated to support timezones having a specific WITH_TIMEZONE seems like a good idea to me as it will be a better experience

I think this PR needs

Get the type correct (I left a comment about how to do this)

Add an entry to WITH_TIMEZONE the user documentation (e.g. https://github.com/apache/arrow-datafusion/blob/f00ef9d5484039dc7013af5b8fa04f7b29ffaba7/docs/source/user-guide/sql/scalar_functions.md?plain=1#L1208
)

Hi @alamb thank you for the suggestion.

does it mean that i need to move WITH_TIMEZONE out of BuiltinScalarFunction ?

alamb · 2023-04-07T14:18:33Z

does it mean that i need to move WITH_TIMEZONE out of BuiltinScalarFunction ?

I am not sure to be honest -- there might not be a good existing pattern to follow for such functions. 🤔

waitingkuo · 2023-04-07T15:18:40Z

@alamb thanks, i'll think about how to fix this

alamb · 2024-04-08T21:08:34Z

Since this has been open for more than a year, closing it down. Feel free to reopen if/when you keep working on it.

add with_timezone

b529afe

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Apr 7, 2023

waitingkuo commented Apr 7, 2023

View reviewed changes

alamb reviewed Apr 7, 2023

View reviewed changes

waitingkuo marked this pull request as draft April 7, 2023 14:02

alamb closed this Apr 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add with_timezone #5906

add with_timezone #5906

waitingkuo commented Apr 7, 2023

waitingkuo left a comment

waitingkuo Apr 7, 2023

alamb Apr 7, 2023

waitingkuo Apr 7, 2023

alamb left a comment

alamb Apr 7, 2023

waitingkuo commented Apr 7, 2023

tustvold commented Apr 7, 2023 •

edited

Loading

waitingkuo commented Apr 7, 2023

alamb commented Apr 7, 2023 •

edited

Loading

waitingkuo commented Apr 7, 2023

alamb commented Apr 8, 2024

add with_timezone #5906

add with_timezone #5906

Conversation

waitingkuo commented Apr 7, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

waitingkuo left a comment

Choose a reason for hiding this comment

waitingkuo Apr 7, 2023

Choose a reason for hiding this comment

alamb Apr 7, 2023

Choose a reason for hiding this comment

waitingkuo Apr 7, 2023

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Apr 7, 2023

Choose a reason for hiding this comment

waitingkuo commented Apr 7, 2023

tustvold commented Apr 7, 2023 • edited Loading

waitingkuo commented Apr 7, 2023

alamb commented Apr 7, 2023 • edited Loading

waitingkuo commented Apr 7, 2023

alamb commented Apr 8, 2024

tustvold commented Apr 7, 2023 •

edited

Loading

alamb commented Apr 7, 2023 •

edited

Loading