waitingkuo commented on code in PR #5906:
URL: https://github.com/apache/arrow-datafusion/pull/5906#discussion_r1160494954
##########
datafusion/physical-expr/src/functions.rs:
##########
@@ -169,6 +169,22 @@ pub fn create_physical_expr(
)))))
})
}
+ BuiltinScalarFunction::WithTimezone => {
+ // function::return_type doesn't consider the arg value which
contains the timezone info
+ // i.e. it could only output Timestamp<TimeUnit, None> or
Timestamo<TimeUnit, Some(SomeFixedTimestamp))
+ // here we parse the second arg as timestamp and modify the return
data_type
+ 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",
+ )))
+ };
Review Comment:
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
##########
datafusion/expr/src/function.rs:
##########
@@ -262,6 +262,25 @@ pub fn return_type(
BuiltinScalarFunction::ArrowTypeof => Ok(DataType::Utf8),
+ 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(),
+ )),
+ },
Review Comment:
```rust
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
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
