Feature/support timestamp plus minus interval #3110

JasonLi-cn · 2022-08-12T02:35:14Z

Which issue does this PR close?

Rationale for this change

Support Timestamp +/- Interval

What changes are included in this PR?

Update DateIntervalExpr‘ evaluate() function mainly:
https://github.com/apache/arrow-datafusion/blob/b80c85359bdfc6d483cb2f222a988a9125ed8670/datafusion/physical-expr/src/expressions/datetime.rs#L88-L160

Are there any user-facing changes?

codecov-commenter · 2022-08-12T04:16:26Z

Codecov Report

Merging #3110 (51888a1) into master (9956f80) will decrease coverage by 0.01%.
The diff coverage is 74.07%.

@@            Coverage Diff             @@
##           master    #3110      +/-   ##
==========================================
- Coverage   85.95%   85.93%   -0.02%     
==========================================
  Files         290      290              
  Lines       52260    52356      +96     
==========================================
+ Hits        44919    44993      +74     
- Misses       7341     7363      +22

Impacted Files	Coverage Δ
datafusion/physical-expr/src/planner.rs	`92.36% <ø> (ø)`
...tafusion/physical-expr/src/expressions/datetime.rs	`82.20% <73.33%> (-4.60%)`	⬇️
datafusion/expr/src/binary_rule.rs	`84.70% <100.00%> (+0.08%)`	⬆️
datafusion/optimizer/src/eliminate_limit.rs	`100.00% <0.00%> (ø)`
...atafusion/physical-expr/src/aggregate/array_agg.rs	`95.91% <0.00%> (+0.04%)`	⬆️
datafusion/expr/src/logical_plan/plan.rs	`77.60% <0.00%> (+0.17%)`	⬆️
datafusion/core/src/physical_plan/metrics/value.rs	`87.43% <0.00%> (+0.50%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

alamb

Thanks @JasonLi-cn - this is a great contribution

The only thing I would like to see are some "integration" level SQL tests of this feature -- perhaps we could extend the test / add some new tests in https://github.com/apache/arrow-datafusion/blob/master/datafusion/core/tests/sql/timestamp.rs

cc @avantgardnerio who added the initial version of this functionality in #2797

datafusion/physical-expr/src/expressions/datetime.rs

avantgardnerio

Just some nits. The functional change looks great, ty!

datafusion/physical-expr/src/expressions/datetime.rs

datafusion/core/tests/sql/timestamp.rs

datafusion/physical-expr/src/expressions/datetime.rs

avantgardnerio · 2022-08-15T15:04:44Z

datafusion/physical-expr/src/expressions/datetime.rs

+) -> Result<ColumnarValue> {
+    let ret = match array.data_type() {
+        DataType::Date32 => {
+            let array = array.as_any().downcast_ref::<Date32Array>().unwrap();


I know this shouldn't panic, but since this is returning a result anyway, would it improve auditability to use the error propagation operator here instead?

Like this?

let array = array.as_any().downcast_ref::<Date32Array>().ok_or( DataFusionError::Execution("Cast Date32Array error".to_string()), )?;

It's a bit more verbose, but I think it makes it easier to tell that it will never panic. @andygrove @alamb do you have thoughts on this?

I think by this point in the plan it is ok to panic! as it indicates a bug in the code and I think having to always do the ceremony to check the type makes it less readable. Maybe we could make a function like as_date32_array that returned an error (similar to the arrow ones that panic?): https://docs.rs/arrow/20.0.0/arrow/array/index.html#functions ?

Sadly it appears we are inconsistent in the codebase with this regards: https://github.com/apache/arrow-datafusion/search?l=Rust&q=downcast 😢

But in other words, I think this PR is ok as written.

I will file a ticket as this is confusing and it would be nice to get DataFusion consistent

Filed #3152

alamb

I think this looks great @JasonLi-cn so I plan to merge it. There are some good potential follow ons (like upstreaming the date/time arithmetic to arrow as well as standardizing the handling of "what to do when downcasting and array fails") but I think they would be best done in a follow on PR.

datafusion/core/tests/sql/timestamp.rs

alamb · 2022-08-15T17:29:32Z

datafusion/physical-expr/src/expressions/datetime.rs

+) -> Result<ColumnarValue> {
+    let ret = match array.data_type() {
+        DataType::Date32 => {
+            let array = array.as_any().downcast_ref::<Date32Array>().unwrap();


I think by this point in the plan it is ok to panic! as it indicates a bug in the code and I think having to always do the ceremony to check the type makes it less readable. Maybe we could make a function like as_date32_array that returned an error (similar to the arrow ones that panic?): https://docs.rs/arrow/20.0.0/arrow/array/index.html#functions ?

Sadly it appears we are inconsistent in the codebase with this regards: https://github.com/apache/arrow-datafusion/search?l=Rust&q=downcast 😢

alamb · 2022-08-15T17:29:41Z

datafusion/physical-expr/src/expressions/datetime.rs

+) -> Result<ColumnarValue> {
+    let ret = match array.data_type() {
+        DataType::Date32 => {
+            let array = array.as_any().downcast_ref::<Date32Array>().unwrap();


But in other words, I think this PR is ok as written.

datafusion/physical-expr/src/expressions/datetime.rs

alamb · 2022-08-15T17:48:00Z

I plan to merge this PR in when the checks pass

ursabot · 2022-08-15T18:12:04Z

Benchmark runs are scheduled for baseline = 3666305 and contender = 6509d0d. 6509d0d 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

jasonnnli added 2 commits August 12, 2022 10:26

support Timestamp +/- Interval

0689d07

restore example parquet_sql

51888a1

github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates labels Aug 12, 2022

JasonLi-cn closed this Aug 12, 2022

JasonLi-cn reopened this Aug 12, 2022

JasonLi-cn mentioned this pull request Aug 12, 2022

feat: support Timestamp +/- Interval #3103

Closed

alamb mentioned this pull request Aug 12, 2022

Add support for adding intervals to dates apache/arrow-rs#2031

Merged

alamb reviewed Aug 12, 2022

View reviewed changes

datafusion/physical-expr/src/expressions/datetime.rs Show resolved Hide resolved

datafusion/physical-expr/src/expressions/datetime.rs Show resolved Hide resolved

datafusion/physical-expr/src/expressions/datetime.rs Outdated Show resolved Hide resolved

support date and timestamp array +/- interval scalar

1a00703

github-actions bot added the core Core DataFusion crate label Aug 15, 2022

avantgardnerio approved these changes Aug 15, 2022

View reviewed changes

alamb approved these changes Aug 15, 2022

View reviewed changes

alamb mentioned this pull request Aug 15, 2022

Rename DateIntervalExpr to DateTimeIntervalExpr #3150

Merged

alamb merged commit 6509d0d into apache:master Aug 15, 2022

avantgardnerio mentioned this pull request Aug 15, 2022

Remove date code no longer needed since arrow-rs PR 2031 merge #3153

Closed

JasonLi-cn mentioned this pull request Aug 16, 2022

Rename do_data_time_math() to do_date_time_math() #3172

Closed

JasonLi-cn deleted the feature/support_timestamp_plus_minus_interval branch August 16, 2022 02:06

JasonLi-cn restored the feature/support_timestamp_plus_minus_interval branch August 16, 2022 03:56

JasonLi-cn deleted the feature/support_timestamp_plus_minus_interval branch August 16, 2022 04:50

Feature/support timestamp plus minus interval #3110

Feature/support timestamp plus minus interval #3110

Uh oh!

Conversation

JasonLi-cn commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

codecov-commenter commented Aug 12, 2022

Codecov Report

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

avantgardnerio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb commented Aug 15, 2022

Uh oh!

ursabot commented Aug 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JasonLi-cn commented Aug 12, 2022 •

edited

Loading