Support type coercion for timestamp and utf8 #4312

andre-cc-natzka · 2022-11-21T19:15:07Z

Which issue does this PR close?

Closes #4311.

Rationale for this change

Currently DataFusion supports type coercion for (DataType::Time, DataType::Utf8) and (DataType::Date, DataType::Utf8) pairs, but is still missing type coercion for (DataType::Timestamp, DataType::Utf8).

What changes are included in this PR?

Two lines of code are added in the temporal_coercion function from datafusion/expr/src/type_coercion/binary.rs to account for type coercion for a (DataType::Timestamp, DataType::Utf8) pair. The output type is DataType::Timestamp(TimeUnit::Nanosecond, _), because it is the only time unit supported by Arrow, which forces us to change the original time unit to nanoseconds. Although not ideal, this is not a problem, as both the left and right hand side of an expression are converted into DataType::Timestamp(TimeUnit::Nanosecond, _) and the expression can be properly evaluated.

Are these changes tested?

Yes, corresponding test cases are added to the test function test_type_coercion from datafusion/expr/src/type_coercion/binary.rs.

Are there any user-facing changes?

No.

This reverts commit a46b97e.

…afusion

…esponding test cases

waitingkuo

thank you @andre-cc-natzka

here's my comments:

rebase master, your time32/64 pr is merged, thanks again
i'll suggest align the text with the prettyprint result (what you see in datafusion-cli)
i.e.
align the result for

❯ select now()::text;
+-----------------------------------+
| now()                             |
+-----------------------------------+
| 2022-11-21 21:42:52.196874 +00:00 |
+-----------------------------------+

to

❯ select now();
+----------------------------------+
| now()                            |
+----------------------------------+
| 2022-11-21T21:42:48.800795+00:00 |
+----------------------------------+

and

❯ select now()::timestamp::text;
+----------------------------+
| now()                      |
+----------------------------+
| 2022-11-21 21:44:48.386022 |
+----------------------------+
1 row in set. Query took 0.003 seconds.

to

❯ select now()::timestamp;
+----------------------------+
| now()                      |
+----------------------------+
| 2022-11-21T21:44:52.195395 |
+----------------------------+

delete the generated datafusion.rs in the proto folder, it's generated by the prost crate https://github.com/tokio-rs/prost

waitingkuo · 2022-11-21T21:35:41Z

datafusion/core/tests/sql/timestamp.rs

@@ -1555,7 +1555,7 @@ async fn cast_timestamp_to_timestamptz() -> Result<()> {
 #[tokio::test]
 async fn test_cast_to_time() -> Result<()> {
    let ctx = SessionContext::new();
-    let sql = "SELECT 0::TIME";
+    let sql = "SELECT 0::TIME64";


i'm not sure why do we have this. but select 0::time64 doesn't work for me

waitingkuo · 2022-11-21T21:36:46Z

datafusion/proto/proto/datafusion.proto

        ScalarTime64Value time64_value = 30;
        IntervalMonthDayNanoValue interval_month_day_nano = 31;
        StructValue struct_value = 32;
        ScalarFixedSizeBinary fixed_size_binary_value = 34;
+>>>>>>> upstream/master


Suggested change

>>>>>>> upstream/master

waitingkuo · 2022-11-21T21:39:46Z

datafusion/proto/proto/datafusion.proto

+message ScalarTime32Value {
+  oneof value {
+    int32 time32_second_value = 1;
+    int32 time32_millisecond_value = 2;
+  };
+}
+
+message ScalarTime64Value {
+  oneof value {
+    int64 time64_microsecond_value = 1;
+    int64 time64_nanosecond_value = 2;
+  };
+}
+


i think you should rebase master, the pr for time32/64 is merged

waitingkuo · 2022-11-21T21:41:22Z

datafusion/core/tests/sql/select.rs

+    // filtering with Time32 and Time64 types
+


not quite sure what it is, i guess we need to delete ths

alamb · 2022-11-23T18:02:08Z

Marking this PR as draft as it still needs some work prior to passing CI

…esponding test cases

…//github.com/andre-cc-natzka/arrow-datafusion into Support_type_coercion_for_Timestamp_and_Utf8

andre-cc-natzka · 2022-12-05T09:06:05Z

Hello @waitingkuo, @alamb. Thanks for your feedback!

It's been a long time, I apologize for that. I took some days of vacation actually. I am also sorry for the state of this PR, I should indeed have rebased master in advance. I have been through the process now and I believe the problems pointed out by @waitingkuo have been solved.

In fact, the changes I implemented here are very simple, and are limited to the ones in datafusion/expr/src/type_coercion/binary.rs, which aim at enabling type coercion to a timestamp (to nanosecond accuracy, since it is the only one supported by Arrow) when a string and a timestamp (with arbitrary precision) are provided. This could be very useful in my opinion.

Thank you again,
André.

alamb

The code looks good to me. Thank you @andre-cc-natzka

I would recommend (perhaps as a follow on) to add some SQL level tests but since there is coverage here I think it looks good to me

waitingkuo · 2022-12-06T08:03:25Z

hi @andre-cc-natzka thank you, it looks good to me.

I suggest that add some SQL level tests

e.g.

❯ select (timestamp '2000-01-01T00:00:00') = '2000-01-01T00:00:00';
+-----------------------------------------------------------+
| Utf8("2000-01-01T00:00:00") = Utf8("2000-01-01T00:00:00") |
+-----------------------------------------------------------+
| true                                                      |
+-----------------------------------------------------------+
1 row in set. Query took 0.002 seconds.

and this should raise error until we could cast TimestampTz to Utf8

❯ select (timestamptz '2000-01-01T00:00:00+00:00') = '2000-01-01T00:00:00+08:00';
NotImplemented("Unsupported CAST from Utf8 to Timestamp(Nanosecond, Some(\"+00:00\"))")

alamb · 2022-12-07T18:31:00Z

I plan to write some SQL level tests for this feature as they will help us in IOx.

alamb · 2022-12-07T19:06:37Z

Thanks again @andre-cc-natzka -- here are some SQL level tests #4545

ursabot · 2022-12-07T19:15:34Z

Benchmark runs are scheduled for baseline = fbadebb and contender = 8547fd8. 8547fd8 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

andre-cc-natzka · 2022-12-08T10:45:51Z

Hello again @alamb , @waitingkuo. Thank you very much for your second revision and for merging the PR!

Thank you @alamb also for adding the SQL tests that test this new functionality, they look really nice!

All the best,
André.

alamb · 2022-12-08T12:14:27Z

Thank you for the contribution @andre-cc-natzka

waitingkuo · 2022-12-08T15:29:09Z

thank you @andre-cc-natzka

andre-cc-natzka and others added 12 commits November 4, 2022 09:19

Support Time32 and Time64 for Type Coercion

a46b97e

Revert "Support Time32 and Time64 for Type Coercion"

ba582b1

This reverts commit a46b97e.

Implement Time32 and Time64 in hash_join and hash_util

1b5b96b

Add review comments

09cc239

Qualify TimeUnits

d589a56

Changes in proto to provide full support for Time32 and Time64

596e384

Add test to ensure Time32 and Time64 are fully supported

8b8e075

First attempt to merge support time32 and time64

645b93d

Merge remote-tracking branch 'upstream/master'

81238e1

Merge branch 'apache:master' into master

5115862

Merge branch 'master' of https://github.com/andre-cc-natzka/arrow-dat…

d8facc7

…afusion

Add support for type coercion for pair (Timestamp, Utf8) and add corr…

60f0084

…esponding test cases

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions labels Nov 21, 2022

andre-cc-natzka mentioned this pull request Nov 21, 2022

Full support for time32 and time64 literal values (ScalarValue) #4156

Merged

waitingkuo reviewed Nov 21, 2022

View reviewed changes

alamb marked this pull request as draft November 23, 2022 18:02

andre-cc-natzka added 12 commits December 5, 2022 08:20

Implement Time32 and Time64 in hash_join and hash_util

9a9e6ea

Add review comments

8407dcc

Changes in proto to provide full support for Time32 and Time64

05f41c3

Add test to ensure Time32 and Time64 are fully supported

68b1200

Add support for type coercion for pair (Timestamp, Utf8) and add corr…

f28308c

…esponding test cases

Merge branch 'Support_type_coercion_for_Timestamp_and_Utf8' of https:…

875a06b

…//github.com/andre-cc-natzka/arrow-datafusion into Support_type_coercion_for_Timestamp_and_Utf8

Revert to_proto.rs

7291260

Revert to_proto.rs

b7e1313

Revert timestamp.rs

726f05d

Revert mod.rs

b5d529d

Revert select.rs

b46a3d0

Revert group_by.rs

85c29bf

andre-cc-natzka added 2 commits December 5, 2022 09:49

Revert aggregates.rs

01a2d6f

Delete generated file

a4d4b45

github-actions bot removed the core Core DataFusion crate label Dec 5, 2022

andre-cc-natzka marked this pull request as ready for review December 5, 2022 09:38

andre-cc-natzka requested a review from waitingkuo December 5, 2022 09:38

alamb approved these changes Dec 5, 2022

View reviewed changes

alamb merged commit 8547fd8 into apache:master Dec 7, 2022

alamb mentioned this pull request Dec 7, 2022

Add tests for coercion of timestamps to strings #4545

Merged

bseifert-natzka mentioned this pull request Feb 2, 2023

Support type coercion for timestamp and utf8 NatzkaLabs/arrow-datafusion-v14-patched#4

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support type coercion for timestamp and utf8 #4312

Support type coercion for timestamp and utf8 #4312

andre-cc-natzka commented Nov 21, 2022 •

edited

Loading

waitingkuo left a comment

waitingkuo Nov 21, 2022

waitingkuo Nov 21, 2022

waitingkuo Nov 21, 2022

waitingkuo Nov 21, 2022

alamb commented Nov 23, 2022

andre-cc-natzka commented Dec 5, 2022

alamb left a comment

waitingkuo commented Dec 6, 2022

alamb commented Dec 7, 2022

alamb commented Dec 7, 2022

ursabot commented Dec 7, 2022

andre-cc-natzka commented Dec 8, 2022

alamb commented Dec 8, 2022

waitingkuo commented Dec 8, 2022

Support type coercion for timestamp and utf8 #4312

Support type coercion for timestamp and utf8 #4312

Conversation

andre-cc-natzka commented Nov 21, 2022 • edited Loading

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 Nov 21, 2022

Choose a reason for hiding this comment

waitingkuo Nov 21, 2022

Choose a reason for hiding this comment

waitingkuo Nov 21, 2022

Choose a reason for hiding this comment

waitingkuo Nov 21, 2022

Choose a reason for hiding this comment

alamb commented Nov 23, 2022

andre-cc-natzka commented Dec 5, 2022

alamb left a comment

Choose a reason for hiding this comment

waitingkuo commented Dec 6, 2022

alamb commented Dec 7, 2022

alamb commented Dec 7, 2022

ursabot commented Dec 7, 2022

andre-cc-natzka commented Dec 8, 2022

alamb commented Dec 8, 2022

waitingkuo commented Dec 8, 2022

andre-cc-natzka commented Nov 21, 2022 •

edited

Loading