Feature/support timestamp plus minus interval #3110
Conversation
github-actions
bot
added
logical-expr
Codecov Report
@@ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
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
| ) -> Result<ColumnarValue> { | ||
| let ret = match array.data_type() { | ||
| DataType::Date32 => { | ||
| let array = array.as_any().downcast_ref::<Date32Array>().unwrap(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Like this?
let array = array.as_any().downcast_ref::<Date32Array>().ok_or(
DataFusionError::Execution("Cast Date32Array error".to_string()),
)?;There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 😢
There was a problem hiding this comment.
But in other words, I think this PR is ok as written.
There was a problem hiding this comment.
I will file a ticket as this is confusing and it would be nice to get DataFusion consistent
There was a problem hiding this comment.
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.
| ) -> Result<ColumnarValue> { | ||
| let ret = match array.data_type() { | ||
| DataType::Date32 => { | ||
| let array = array.as_any().downcast_ref::<Date32Array>().unwrap(); |
There was a problem hiding this comment.
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 😢
| ) -> Result<ColumnarValue> { | ||
| let ret = match array.data_type() { | ||
| DataType::Date32 => { | ||
| let array = array.as_any().downcast_ref::<Date32Array>().unwrap(); |
There was a problem hiding this comment.
But in other words, I think this PR is ok as written.
|
I plan to merge this PR in when the checks pass |
|
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. |
Which issue does this PR close?
Closes #3103 (comment)
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?