Date to Timestamp cast #5140

comphead · 2023-02-01T00:55:37Z

Which issue does this PR close?

Closes #4761
Closes #4644 .

Rationale for this change

Introduce casting between dates and timestamps datatypes. Currently DF just panics on such kind of queries

What changes are included in this PR?

This PR includes planner changes to support date to timestamp cast.

Are these changes tested?

Yes. Tests enclosed

Are there any user-facing changes?

No

comphead · 2023-02-01T00:57:33Z

@liukun4515 @crepererum @alamb please check this one.

The downside for now is additional cast created sometimes for timestamp, to convert Timestamp(_, Some("00:00")) to Timestamp(_, None). This can be solved in separate PR if you dont mind as it might again require changes in arrow-rs

alamb

Looks great -- thank you @comphead

cc @waitingkuo (we are getting quite sophisticated in DataFusion for dates/times)

alamb · 2023-02-01T12:10:20Z

datafusion/core/tests/sql/timestamp.rs

@@ -1742,5 +1741,55 @@ async fn test_ts_dt_binary_ops() -> Result<()> {

    assert_batches_eq!(expected, &results);

+    //test cast path timestamp date using literals
+    let sql = "select '2000-01-01'::timestamp >= '2000-01-01'::date";


You might be able to add these tests to sqllogictest if you wanted -- https://github.com/apache/arrow-datafusion/blob/master/datafusion/core/tests/sqllogictests/test_files/timestamps.slt

Not required but I find the sqllogictest framework much easier to work with

Thanks @alamb I will create a separate ticket to move all timestamp tests from timestamp.rs to sqllogictest, all that can be moved of course

Thank you very much -- that would be great

Filed #5165

alamb · 2023-02-01T12:11:30Z

datafusion/expr/src/type_coercion/functions.rs

-        Timestamp(TimeUnit::Nanosecond, None) => {
-            matches!(type_from, Null | Timestamp(_, None))
+        Timestamp(TimeUnit::Nanosecond, _) => {
+            matches!(type_from, Null | Timestamp(_, _) | Date32)


👍 (I have a draft PR in the works to support coercing from utf8 as well #5117)

alamb · 2023-02-01T12:12:55Z

datafusion/core/tests/sql/timestamp.rs

+            break;
+        }
+    }
+    assert_eq!(res, Some("Projection: CAST(now() AS Timestamp(Nanosecond, None)) = CAST(currentdate() AS Timestamp(Nanosecond, None))\n  EmptyRelation".to_string()));


alamb · 2023-02-01T12:14:45Z

cc @crepererum and @liukun4515 who filed the tickets this closes

alamb · 2023-02-01T14:22:41Z

The downside for now is additional cast created sometimes for timestamp, to convert Timestamp(, Some("00:00")) to Timestamp(, None). This can be solved in separate PR if you dont mind as it might again require changes in arrow-rs

I think this is correct (and needed) if the timestamps in question have different timezones. It should not really effect runtime performance as I expect both to be evaluated at plan time (and we can add a constant folding rule if it is not)

alamb · 2023-02-01T22:21:36Z

I plan to merge this PR tomorrow unless there are other comments

waitingkuo

thank you @comphead
left the comments bellow

I think we could consider adding this coercion as well as it works in postgrseql (perhaps in another tickets)
(Utf8, Date)

postgrseql

willy=# select '2000-01-01T00:00:00' = '2000-01-01'::date;
 ?column? 
----------
 t
(1 row)

datafusion

❯ select '2000-01-01T00:00:00' = '2000-01-01'::date;
ArrowError(CastError("Cannot cast string '2000-01-01T00:00:00' to value of Date32 type"))

waitingkuo · 2023-02-02T09:05:46Z

datafusion/expr/src/type_coercion/binary.rs

+        (Timestamp(_, tz), Utf8) | (Utf8, Timestamp(_, tz)) => {
+            Some(Timestamp(TimeUnit::Nanosecond, tz.clone()))
+        }


i tried this, but it didn't work

❯ select '2000-01-01T00:00:00'::timestamp::timestamptz = '2000-01-01T00:00:00'; Internal("The type of Timestamp(Nanosecond, Some(\"+00:00\")) Eq Utf8 of binary physical should be same")

We need another cast to add Utf <-> Timestamp #5117 probably addresses this cast. If not I'll create another ticket

Even after #5117 this still fails. I will file a follow on ticket

❯ select '2000-01-01T00:00:00'::timestamp::timestamptz = '2000-01-01T00:00:00';
Internal("The type of Timestamp(Nanosecond, Some("+00:00")) Eq Utf8 of binary physical should be same")

Filed #5164

waitingkuo · 2023-02-02T09:13:40Z

datafusion/expr/src/type_coercion/binary.rs

+        (Timestamp(_, None), Date32) | (Date32, Timestamp(_, None)) => {
+            Some(Timestamp(TimeUnit::Nanosecond, None))
+        }
+        (Timestamp(_, _tz), Date32) | (Date32, Timestamp(_, _tz)) => {
+            Some(Timestamp(TimeUnit::Nanosecond, None))
+        }


does it mean that we convert Timestamptz to Timestamp and then compare?

Thats correct, we lose TZ for now, we need some extra efforts to make arrow-rs work with TimestampTZ,planning to do so just after this PR merged.

alamb · 2023-02-02T14:13:46Z

I think we could consider adding this coercion as well as it works in postgrseql (perhaps in another tickets)
(Utf8, Date)

I agree -- I can do so.

alamb · 2023-02-02T17:12:44Z

We need another cast to add Utf <-> Timestamp #5117 probably addresses this cast. If not I'll create another ticket

I am going to merge this PR and test using #5117 and if it doesn't work I'll file another ticket

ursabot · 2023-02-02T17:21:58Z

Benchmark runs are scheduled for baseline = f0c0958 and contender = e41c4df. e41c4df 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

alamb · 2023-02-02T17:29:43Z

Filed #5164

comphead added 3 commits January 24, 2023 17:16

ts to date cast

887c8f6

Merge branch 'master' into ts-date-cast

17f50a5

Date to Timestamp Cast

ca350c1

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules labels Feb 1, 2023

alamb approved these changes Feb 1, 2023

View reviewed changes

comphead mentioned this pull request Feb 1, 2023

Change coerced type for comparison between timestamp with date to timestamp #4761

Closed

waitingkuo reviewed Feb 2, 2023

View reviewed changes

alamb merged commit e41c4df into apache:master Feb 2, 2023

alamb mentioned this pull request Feb 2, 2023

Supporting casts to timestamptz: Eq Utf8 of binary physical should be same" #5164

Closed

comphead mentioned this pull request Feb 2, 2023

Move tests from timestamp.rs to sqllogictest 'timestamps.slt' #5165

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Date to Timestamp cast #5140

Date to Timestamp cast #5140

comphead commented Feb 1, 2023 •

edited by alamb

Loading

comphead commented Feb 1, 2023

alamb left a comment

alamb Feb 1, 2023

comphead Feb 1, 2023

alamb Feb 2, 2023

comphead Feb 2, 2023

alamb Feb 1, 2023

alamb Feb 1, 2023

alamb commented Feb 1, 2023

alamb commented Feb 1, 2023 •

edited

Loading

alamb commented Feb 1, 2023

waitingkuo left a comment

waitingkuo Feb 2, 2023

comphead Feb 2, 2023

alamb Feb 2, 2023

alamb Feb 2, 2023

waitingkuo Feb 2, 2023

comphead Feb 2, 2023

alamb commented Feb 2, 2023

alamb commented Feb 2, 2023

ursabot commented Feb 2, 2023

alamb commented Feb 2, 2023

Date to Timestamp cast #5140

Date to Timestamp cast #5140

Conversation

comphead commented Feb 1, 2023 • edited by alamb 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?

comphead commented Feb 1, 2023

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Feb 1, 2023

alamb commented Feb 1, 2023 • edited Loading

alamb commented Feb 1, 2023

waitingkuo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Feb 2, 2023

alamb commented Feb 2, 2023

ursabot commented Feb 2, 2023

alamb commented Feb 2, 2023

comphead commented Feb 1, 2023 •

edited by alamb

Loading

alamb commented Feb 1, 2023 •

edited

Loading