Add to_unixtime function
#9077
Add to_unixtime function
#9077
Conversation
github-actions
bot
added
logical-expr
Tangruilin
force-pushed
the
task#5568
branch
3 times, most recently
from
a7da055 to
eab02b2
Compare
|
Now that @Omega359 started moving the date time functions to |
I see that. I will do this tomorrow |
|
Thank you @Tangruilin |
github-actions
bot
removed
logical-expr
Tangruilin
force-pushed
the
task#5568
branch
2 times, most recently
from
10fb2d4 to
91f3eb6
Compare
This PR is ready to review |
There was a problem hiding this comment.
Thanks @Tangruilin -- this is looking like a good start. Perhaps @waynexia has some thoughts on expected behavior and time to review given he filed #5568
Could you please also add an entry for to_unixtime in the documentation? https://github.com/apache/arrow-datafusion/blob/main/docs/source/user-guide/sql/scalar_functions.md#to_timestamp
| @@ -0,0 +1,60 @@ | |||
| // Licensed to the Apache Software Foundation (ASF) under one | |||
There was a problem hiding this comment.
FWIW I don't think we need to make a to_unixtime example -- I think the other examples for using the functions are plenty thorough. In fact I recommend we remove this example so it doesn't become a pattern for all newly added datetime functions ( could do this as a follow on PR )
|
|
||
| fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> { | ||
| if args.is_empty() { | ||
| return exec_err!("to_date function requires 1 or more arguments, got 0"); |
There was a problem hiding this comment.
| return exec_err!("to_date function requires 1 or more arguments, got 0"); | |
| return exec_err!("to_unixtime function requires 1 or more arguments, got 0"); |
| | DataType::Null | ||
| | DataType::Float64 | ||
| | DataType::Date32 | ||
| | DataType::Date64 => args[0].cast_to(&DataType::Int64, None), |
There was a problem hiding this comment.
These types look a little strange to me. Why is Int32/Int64/Float supported? If that is intentional, can you please add tests of calling to_unixtime with arguments of those types?
Also, the spark to_unixtime function https://spark.apache.org/docs/2.3.0/api/sql/index.html#to_unix_timestamp seems to also accept timestamps (DateTime::Timestamp(..)) -- should this function do so as well?
Also I think casting a Date32 to an Int64 will yield the number of seconds
https://docs.rs/arrow/latest/arrow/datatypes/enum.DataType.html#variant.Date32
However, casting a Date64 to an Int64 I think will yield the number of milliseconds since the unix epoch https://docs.rs/arrow/latest/arrow/datatypes/enum.DataType.html#variant.Date64
which doesn't sound correct.
Perhaps you can write a test like the following to be sure:
select to_unixtime(arrow_cast('2020-09-08T12:00:00+00:00', 'Date64'));There was a problem hiding this comment.
Good Suggestion，I will do it this week
Tangruilin
force-pushed
the
task#5568
branch
3 times, most recently
from
eff4770 to
79889b1
Compare
Signed-off-by: tangruilin <tang.ruilin@foxmail.com>
| if args.len() > 1 { | ||
| if let Some(value) = validate_data_types(args, "to_unixtime") { | ||
| return value; | ||
| } | ||
| } |
There was a problem hiding this comment.
I should say the validate_data_types is a bit misleading... It won't return Some(Ok(_)). We can submit a follow-up PR to change its signature to (...) -> Option<Result<()>> to avoid confusion. Here we can use value? instead of returning it.
| } | ||
| DataType::Date64 | DataType::Date32 | DataType::Timestamp(_, None) => args[0] | ||
| .cast_to(&DataType::Timestamp(TimeUnit::Second, None), None)? | ||
| .cast_to(&DataType::Int64, None), |
|
I merged up from main to resolve the CI failures |
|
Thanks again @Tangruilin and @waynexia |
Which issue does this PR close?
Closes #5568 .
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?