Add support for month & year intervals #2797

avantgardnerio · 2022-06-26T22:04:05Z

Which issue does this PR close?

Closes #2796.

Rationale for this change

In order to pass TPC-H benchmarks, datafusion will need to be able to support month & year date intervals.

What changes are included in this PR?

A refactoring of the datetime module to:

Support IntervalYearMonth in addition to the currently supported IntervalDayTime
Perform some non-trivial date modulo math (that I hopefully got correct)
Add tests to ensure modulo math edge cases work correctly
Switch to an arguably more readable railroad error handling pattern to take better advantage of the error propagation (?) operator.

Are there any user-facing changes?

More queries should work. Nothing that worked previously should break.

codecov-commenter · 2022-06-26T23:24:13Z

Codecov Report

Merging #2797 (8ba1efe) into master (364c9cc) will increase coverage by 0.09%.
The diff coverage is 93.94%.

@@            Coverage Diff             @@
##           master    #2797      +/-   ##
==========================================
+ Coverage   85.25%   85.34%   +0.09%     
==========================================
  Files         275      276       +1     
  Lines       49001    49281     +280     
==========================================
+ Hits        41774    42061     +287     
+ Misses       7227     7220       -7

Impacted Files	Coverage Δ
datafusion/common/src/scalar.rs	`75.77% <ø> (ø)`
datafusion/physical-expr/src/expressions/mod.rs	`100.00% <ø> (ø)`
...tafusion/physical-expr/src/expressions/datetime.rs	`86.80% <89.44%> (+54.14%)`	⬆️
datafusion/core/tests/sql/timestamp.rs	`100.00% <100.00%> (ø)`
datafusion/optimizer/src/simplify_expressions.rs	`82.00% <100.00%> (+0.02%)`	⬆️
datafusion/physical-expr/src/expressions/delta.rs	`100.00% <100.00%> (ø)`
datafusion/expr/src/logical_plan/plan.rs	`74.31% <0.00%> (-0.20%)`	⬇️
datafusion/expr/src/window_frame.rs	`93.27% <0.00%> (+0.84%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 364c9cc...8ba1efe. Read the comment docs.

alamb

Thank you for the contribution @avantgardnerio -- this is looking like a great start. I don't fully understand some of the calculations but I may be misunderstanding

cc @ovr

datafusion/common/src/scalar.rs

datafusion/core/tests/sql/timestamp.rs

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

alamb · 2022-06-27T18:14:59Z

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

+        };
+
+        // Add interval
+        let posterior = match scalar {


It would b great to eventually put some/all of this logic into the arrow-rs kernels -- for example

https://docs.rs/arrow/16.0.0/arrow/compute/kernels/temporal/index.html

or perhaps https://docs.rs/arrow/16.0.0/arrow/compute/kernels/arithmetic/fn.add.html

That would also likely result in support for columnar execution support (aka adding a column of integers)

Maybe we can (I will file a ticket) start with kernels in datafusion and then port them to arrow.rs

See apache/arrow-rs#2031

datafusion/common/src/scalar.rs

datafusion/core/tests/sql/timestamp.rs

datafusion/optimizer/src/simplify_expressions.rs

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

avantgardnerio · 2022-06-29T16:40:58Z

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

+    NaiveDate::from_ymd(target.year(), target.month(), day)
+}
+
+fn chrono_add_months(dt: NaiveDate, delta: i32) -> NaiveDate {


I still think I would like to PR this to chrono when I get a chance, and we can remove it from datafusion.

👍 - I like filing a ticket / PR in the target repo and then leaving a link in the comments of DataFusion

Btw I poked around and found this code in arrow2 (from @jorgecarleitao ) that is similar: https://docs.rs/arrow2/latest/src/arrow2/temporal_conversions.rs.html#342-368

alamb

Thanks @avantgardnerio I think this PR's code is looking good

The only thing I think it needs before merging are tests covering more of the date/time conversion logic. Once that is done I can write up a "move this to arrow" type ticket as well

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

alamb · 2022-07-02T11:08:30Z

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

+    NaiveDate::from_ymd(target.year(), target.month(), day)
+}
+
+fn chrono_add_months(dt: NaiveDate, delta: i32) -> NaiveDate {


👍 - I like filing a ticket / PR in the target repo and then leaving a link in the comments of DataFusion

Btw I poked around and found this code in arrow2 (from @jorgecarleitao ) that is similar: https://docs.rs/arrow2/latest/src/arrow2/temporal_conversions.rs.html#342-368

avantgardnerio · 2022-07-06T16:23:00Z

Sorry this stalled out. I was really close on the subqueries, and I think I have them all working now, so I'll jump back on this.

avantgardnerio · 2022-07-07T18:10:12Z

Added unit tests. Will file PRs to respective dependency repos as well.

chrono - I can figure out where to send the PR
arrow - any idea what file / impl the functions listed as TODO should go into?

avantgardnerio · 2022-07-07T20:07:44Z

I PRed the chrono repo.

avantgardnerio · 2022-07-07T20:19:45Z

Once that is done I can write up a "move this to arrow" type ticket

@alamb , do you know where (in arrow-rs), I can PR these functions to:

fn create_day_time(days: i32, millis: i32) -> i64
fn create_month_day_nano(months: i32, days: i32, nanos: i64) -> i128

?

avantgardnerio · 2022-07-07T21:44:52Z

or perhaps https://docs.rs/arrow/16.0.0/arrow/compute/kernels/arithmetic/fn.add.html

Okay, I think I understand this. Ideally math_op would look something like this:

pub fn math_op<LT, RT, F>(
    left: &PrimitiveArray<LT>,
    right: &PrimitiveArray<RT>,
    op: F,
) -> Result<PrimitiveArray<LT>>
where
    LT: ArrowNumericType,
    RT: ArrowNumericType,
    F: Fn(LT::Native, RT::Native) -> LT::Native,
{

So that we could add an array of IntervalXXX to and array of DateXXX. Unfortunately, that results in combinatorial hell ATM:

error[E0277]: cannot add `i16` to `i8`
   --> arrow/src/compute/kernels/arithmetic.rs:741:51
    |
741 |         _ => typed_math_op!(left, right, |a, b| a + b),
    |                                                   ^ no implementation for `i8 + i16`
    |
    = help: the trait `Add<i16>` is not implemented for `i8`
    = help: the following other types implement trait `Add<Rhs>`:
              <&'a f32 as Add<Complex<f32>>>
              <&'a f32 as Add<f32>>
              <&'a f64 as Add<Complex<f64>>>
              <&'a f64 as Add<f64>>
              <&'a i128 as Add<BigInt>>
              <&'a i128 as Add<Complex<i128>>>
              <&'a i128 as Add<i128>>
              <&'a i16 as Add<BigInt>>
            and 176 others

🤔

avantgardnerio · 2022-07-07T22:32:15Z

I think I see how to PR this into the arrow kernels. I'll try that shortly.

avantgardnerio · 2022-07-08T01:32:28Z

What this is looking like when done inside arrow-rs: apache/arrow-rs@master...spaceandtimelabs:arrow-rs:bg_date_math

alamb · 2022-07-08T19:25:42Z

@alamb , do you know where (in arrow-rs), I can PR these functions to:

I recommend https://github.com/apache/arrow-rs/blob/master/arrow/src/compute/kernels/temporal.rs

alamb · 2022-07-08T19:26:57Z

For what it is worth apache/arrow-rs@master...spaceandtimelabs:arrow-rs:bg_date_math looks pretty good to me 👍

alamb

I think this is looking quite good -- thank you @avantgardnerio . I like the idea of getting the test coverage and then expanding support by adding kernels to arrow.

alamb · 2022-07-08T19:34:25Z

datafusion/physical-expr/Cargo.toml

@@ -44,6 +44,7 @@ arrow = { version = "17.0.0", features = ["prettyprint"] }
 blake2 = { version = "^0.10.2", optional = true }
 blake3 = { version = "1.0", optional = true }
 chrono = { version = "0.4", default-features = false }
+chronoutil = "0.2.3"


I am somewhat concerned about adding dependencies (and this particular crate seems to be maintained by a single person) -- however, since it is so small (and MIT licensced), we could also just inline the functions we care about.

Any thoughts @andygrove or @thinkharderdev ?

https://github.com/olliemath/chronoutil/blob/main/src/delta.rs

I haven't reviewed the PR so am not sure how much of that create is used but I agree if it is a small amount of code it is better to just copy the code rather than add another dependency.

I inlined it in my latest push. If we want to go back to including it in cargo.toml, it's an easy revert.

My $0.02 would be to leave it in cargo.toml, so that we can get updates if the author fixes something, and so that RAT checkers can see declaratively that we depend on that code, rather than trying to use heuristics of sematic code or binary diff.

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb

I think this is good to go from my perspective. Does anyone else care to review?

cc @ovr this may be interesting / relevant to your project
cc @paddyhoran

alamb · 2022-07-11T18:51:42Z

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

+        };
+
+        // Add interval
+        let posterior = match scalar {


See apache/arrow-rs#2031

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

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb · 2022-07-12T18:33:01Z

🐱 🚀

github-actions bot added core Core datafusion crate physical-expr Physical Expressions labels Jun 26, 2022

andygrove requested review from Dandandan and alamb June 27, 2022 04:23

alamb reviewed Jun 27, 2022

View reviewed changes

avantgardnerio force-pushed the bg_month_interval branch from 10e7b3d to 874a5ed Compare June 29, 2022 15:46

github-actions bot added the optimizer Optimizer rules label Jun 29, 2022

avantgardnerio commented Jun 29, 2022

View reviewed changes

datafusion/common/src/scalar.rs Show resolved Hide resolved

avantgardnerio commented Jun 29, 2022

View reviewed changes

datafusion/core/tests/sql/timestamp.rs Show resolved Hide resolved

avantgardnerio commented Jun 29, 2022

View reviewed changes

datafusion/optimizer/src/simplify_expressions.rs Show resolved Hide resolved

avantgardnerio commented Jun 29, 2022

View reviewed changes

datafusion/optimizer/src/simplify_expressions.rs Show resolved Hide resolved

avantgardnerio commented Jun 29, 2022

View reviewed changes

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

avantgardnerio commented Jun 29, 2022

View reviewed changes

avantgardnerio requested a review from alamb June 29, 2022 16:41

This was referenced Jun 29, 2022

Optimize where exists sub-queries into aggregate and join #2813

Closed

TPC-H Query 4 #160

Closed

alamb reviewed Jul 2, 2022

View reviewed changes

avantgardnerio force-pushed the bg_month_interval branch from 8348470 to 06dbb21 Compare July 6, 2022 20:51

avantgardnerio requested a review from alamb July 7, 2022 18:09

avantgardnerio mentioned this pull request Jul 7, 2022

Create add_months() method on NaiveDate chronotope/chrono#731

Merged

alamb approved these changes Jul 8, 2022

View reviewed changes

alamb mentioned this pull request Jul 8, 2022

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

Merged

avantgardnerio and others added 15 commits July 9, 2022 12:52

Failing tests

0333483

Add month/year arithmetic

7b627f1

Fix tests?

26d73b0

Fix clippy?

620597a

Update datafusion/common/src/scalar.rs

8d310c1

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Add support for all types, fix math

0a77c2c

Fix doc

ed942db

Fix test that relied on previous flawed implementation

01cc9f8

Appease clippy

b16e210

Cleanup

1deb58c

Fix clippy

3546d1f

Links to chrono issue

c473dce

Switch to chronoutil to avoid re-inventing the wheel

a5b30be

Fix linting

8c2007d

Inline chronoutils

8ba1efe

avantgardnerio force-pushed the bg_month_interval branch from dd100d0 to 8ba1efe Compare July 9, 2022 18:52

alamb approved these changes Jul 11, 2022

View reviewed changes

avantgardnerio and others added 4 commits July 11, 2022 12:58

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

72d963d

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

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

154410d

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

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

59f77d3

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

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

d5867c0

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

andygrove approved these changes Jul 12, 2022

View reviewed changes

andygrove merged commit 5a63c87 into apache:master Jul 12, 2022

avantgardnerio deleted the bg_month_interval branch July 12, 2022 17:22

JasonLi-cn mentioned this pull request Aug 12, 2022

Feature/support timestamp plus minus interval #3110

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for month & year intervals #2797

Add support for month & year intervals #2797

avantgardnerio commented Jun 26, 2022

codecov-commenter commented Jun 26, 2022 •

edited

alamb left a comment

alamb Jun 27, 2022

alamb Jul 11, 2022

avantgardnerio Jun 29, 2022

alamb Jul 2, 2022

alamb left a comment

alamb Jul 2, 2022

avantgardnerio commented Jul 6, 2022

avantgardnerio commented Jul 7, 2022

avantgardnerio commented Jul 7, 2022

avantgardnerio commented Jul 7, 2022 •

edited

avantgardnerio commented Jul 7, 2022 •

edited

avantgardnerio commented Jul 7, 2022

avantgardnerio commented Jul 8, 2022

alamb commented Jul 8, 2022

alamb commented Jul 8, 2022

alamb left a comment

alamb Jul 8, 2022

thinkharderdev Jul 8, 2022

avantgardnerio Jul 8, 2022

alamb left a comment

alamb Jul 11, 2022

alamb commented Jul 12, 2022

Add support for month & year intervals #2797

Add support for month & year intervals #2797

Conversation

avantgardnerio commented Jun 26, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

codecov-commenter commented Jun 26, 2022 • edited

Codecov Report

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

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avantgardnerio commented Jul 6, 2022

avantgardnerio commented Jul 7, 2022

avantgardnerio commented Jul 7, 2022

avantgardnerio commented Jul 7, 2022 • edited

avantgardnerio commented Jul 7, 2022 • edited

avantgardnerio commented Jul 7, 2022

avantgardnerio commented Jul 8, 2022

alamb commented Jul 8, 2022

alamb commented Jul 8, 2022

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

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jul 12, 2022

codecov-commenter commented Jun 26, 2022 •

edited

avantgardnerio commented Jul 7, 2022 •

edited

avantgardnerio commented Jul 7, 2022 •

edited