Add `to_unixtime` function by Tangruilin · Pull Request #9077 · apache/datafusion

Tangruilin · 2024-01-31T01:46:45Z

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?

alamb · 2024-03-02T15:55:41Z

Now that @Omega359 started moving the date time functions to datafusion-functions perhaps you can follow the same pattern for to_unixtime 🤔

Tangruilin · 2024-03-02T15:57:44Z

Now that @Omega359 started moving the date time functions to datafusion-functions perhaps you can follow the same pattern for to_unixtime 🤔

I see that.

I will do this tomorrow

alamb · 2024-03-02T19:13:34Z

Thank you @Tangruilin

Tangruilin · 2024-03-03T09:48:11Z

Thank you @Tangruilin

This PR is ready to review

alamb

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

alamb · 2024-03-03T15:00:53Z

datafusion-examples/examples/to_unixtime.rs

@@ -0,0 +1,60 @@
+// Licensed to the Apache Software Foundation (ASF) under one


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 )

alamb · 2024-03-03T15:02:33Z

datafusion/functions/src/datetime/to_unixtime.rs

+
+    fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
+        if args.is_empty() {
+            return exec_err!("to_date function requires 1 or more arguments, got 0");


Suggested change

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");

alamb · 2024-03-03T15:10:02Z

datafusion/functions/src/datetime/to_unixtime.rs

+            | DataType::Null
+            | DataType::Float64
+            | DataType::Date32
+            | DataType::Date64 => args[0].cast_to(&DataType::Int64, None),


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'));

Good Suggestion，I will do it this week

Signed-off-by: tangruilin <[email protected]>

waynexia

The rest LGTM, thanks!

waynexia · 2024-03-07T08:55:57Z

datafusion/functions/src/datetime/to_unixtime.rs

+        if args.len() > 1 {
+            if let Some(value) = validate_data_types(args, "to_unixtime") {
+                return value;
+            }
+        }


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.

alamb

Thank you @Tangruilin and @waynexia

alamb · 2024-03-08T20:55:40Z

datafusion/functions/src/datetime/to_unixtime.rs

+            }
+            DataType::Date64 | DataType::Date32 | DataType::Timestamp(_, None) => args[0]
+                .cast_to(&DataType::Timestamp(TimeUnit::Second, None), None)?
+                .cast_to(&DataType::Int64, None),


alamb · 2024-03-08T20:57:38Z

I merged up from main to resolve the CI failures

alamb · 2024-03-08T22:12:41Z

Thanks again @Tangruilin and @waynexia

Tangruilin marked this pull request as draft January 31, 2024 01:46

github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates labels Jan 31, 2024

Tangruilin force-pushed the task#5568 branch 3 times, most recently from a7da055 to eab02b2 Compare March 2, 2024 09:37

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Mar 2, 2024

Tangruilin force-pushed the task#5568 branch from eab02b2 to 664c4b0 Compare March 2, 2024 09:51

Tangruilin force-pushed the task#5568 branch from 664c4b0 to e03cd1f Compare March 3, 2024 04:00

github-actions bot removed logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates labels Mar 3, 2024

Tangruilin marked this pull request as ready for review March 3, 2024 04:00

Tangruilin force-pushed the task#5568 branch 2 times, most recently from 10fb2d4 to 91f3eb6 Compare March 3, 2024 06:39

alamb reviewed Mar 3, 2024

View reviewed changes

waynexia self-requested a review March 5, 2024 02:24

Tangruilin force-pushed the task#5568 branch 3 times, most recently from eff4770 to 79889b1 Compare March 7, 2024 05:14

[task apache#5568] add_to_unixtime_function

f1afcb6

Signed-off-by: tangruilin <[email protected]>

Tangruilin force-pushed the task#5568 branch from 79889b1 to f1afcb6 Compare March 7, 2024 06:14

waynexia approved these changes Mar 7, 2024

View reviewed changes

waynexia mentioned this pull request Mar 7, 2024

chore: simplify the return type of validate_data_types() #9491

Merged

alamb mentioned this pull request Mar 8, 2024

DataFusion weekly project plan (Andrew Lamb) - March 4, 2024 #9453

Closed

5 tasks

alamb approved these changes Mar 8, 2024

View reviewed changes

alamb changed the title ~~[task #5568] add_to_unixtime_function~~ Add to_unixtime function Mar 8, 2024

Merge remote-tracking branch 'apache/main' into task#5568

d1af339

Fix clippy

2f0a9b8

alamb merged commit 32bb26d into apache:main Mar 8, 2024

		@@ -0,0 +1,60 @@
		// Licensed to the Apache Software Foundation (ASF) under one

	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");

Comments

Conversation

Tangruilin commented Jan 31, 2024

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?

Uh oh!

alamb commented Mar 2, 2024

Uh oh!

Tangruilin commented Mar 2, 2024

Uh oh!

alamb commented Mar 2, 2024

Uh oh!

Tangruilin commented Mar 3, 2024

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Mar 3, 2024

Choose a reason for hiding this comment

Uh oh!

alamb Mar 3, 2024

Choose a reason for hiding this comment

Uh oh!

alamb Mar 3, 2024

Choose a reason for hiding this comment

Uh oh!

Tangruilin Mar 6, 2024

Choose a reason for hiding this comment

Uh oh!

waynexia left a comment

Choose a reason for hiding this comment

Uh oh!

waynexia Mar 7, 2024

Choose a reason for hiding this comment

Uh oh!

waynexia Mar 7, 2024

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

alamb commented Mar 8, 2024

Uh oh!

alamb commented Mar 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants