ARROW-10816: [Rust][DataFusion] Initial support for Interval expressions by ovr · Pull Request #9373 · apache/arrow

ovr · 2021-01-30T16:20:13Z

Initial support for Interval expressions in Data Fusion. This PR allows creating interval from SQL, store intervals in ScalarValue(s) and displaying intervals in tests.

github-actions · 2021-01-30T16:20:33Z

https://issues.apache.org/jira/browse/ARROW-10816

codecov-io · 2021-01-30T18:58:35Z

Codecov Report

Merging #9373 (3e10d6d) into master (3d4c2bb) will decrease coverage by 0.05%.
The diff coverage is 57.85%.

@@ Coverage Diff @@## master #9373 +/- ##
==========================================
- Coverage 82.10% 82.05% -0.06% 
==========================================
Files 230 230 Lines 54043 54164 +121 ==========================================
+ Hits 44374 44442 +68 - Misses 9669 9722 +53

Impacted Files	Coverage Δ
rust/arrow/src/datatypes.rs	`78.33% <ø> (ø)`
rust/datafusion/src/sql/planner.rs	`82.70% <51.11%> (-3.01%)`	⬇️
rust/datafusion/src/scalar.rs	`56.28% <63.15%> (+0.43%)`	⬆️
rust/arrow/src/util/display.rs	`77.14% <100.00%> (+1.38%)`	⬆️
rust/datafusion/tests/sql.rs	`99.85% <100.00%> (+<0.01%)`	⬆️
rust/arrow/src/array/equal/utils.rs	`75.49% <0.00%> (-0.99%)`	⬇️
rust/parquet/src/encodings/encoding.rs	`94.86% <0.00%> (-0.20%)`	⬇️

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 3d4c2bb...7e55dd4. Read the comment docs.

ovr · 2021-01-31T20:20:35Z

@alamb @andygrove @Dandandan @jorgecarleitao It's initial support for intervals, I've got some questions:

EDITED

Is it correct how I store it? (I found in C++ source, starting to work on correct implementation)
What about rang overflows, should it be implemented?

P.S I next PR i will add binary operations for intervals and support for DateTime + Intervals.

Thanks

ovr · 2021-01-31T20:33:10Z

Wow, I found that it's possible to write

select INTERVAL '1 hour 1 second 1 month';
select INTERVAL '250 year 1 month 1 day 100 milliseconds';

Start to work on support for it.

And I found how It's implemented in C++

https://github.com/apache/arrow/blob/master/cpp/src/arrow/type.h#L1238

Started to rework this branch, marked as DRAFT.

alamb

This is looking really nice @ovr -- let me know when you are ready for a full review.

alamb · 2021-02-01T21:38:05Z

might be worth testing with a fractional interval as well interval '2.3' years as I think your implementation parses the numbers as a f32

Thanks, added.

alamb · 2021-02-01T21:38:40Z

👍 is really nice

Dandandan · 2021-02-03T17:24:06Z

I think this should be i32 as well as the other types f32?

I am using f32 for compability with PostgreSQL dialect:
SELECT INTERVAL '0.5 day';
0 years 0 mons 0 days 12 hours 0 mins 0.00 secs

Didn't realize that was possible. What about rounding errors (and rounding in general)?

I dont have any ideas, how it should be correct, but anyway using 0.5 DAY or 0.5 HOUR will be rly usefull and works as expected.
Can you point my attention how it should works?
Thanks

I am not sure whether rounding errors are going to be a problem, I could imagine some calculation might cause for example the number of milliseconds to be off for non-trivial fractions?
In that case we should use some decimal type instead of f32 in order to avoid those rounding errors.

@Dandandan
for example the number of milliseconds to be off for non-trivial fractions?
It's better to use Decimal anyway.
👍 I can place a comment in this place with // @todo, when I will finish with #9232 I can reuse Decimal128 in this place. Is it ok? Thanks

ovr · 2021-02-04T10:57:36Z

I think this PR is now ready for review/merge.

In the next PRs I am planing to work on:

BinaryExpressions for Dates and Intervals
ISO8601 intervals
Introduce ArrowIntervalType trait

Thanks

alamb · 2021-02-04T11:40:59Z

Thanks @ovr -- I plan to review this carefully later today

alamb

Thanks @ovr this is looking really nice.

I have some questions / concerns over NULL and fractional month handling, but otherwise I think this PR looks great.

I think getting this PR to a place where we generate errors when we can't represent the intervals precisely is a good enough place to stop for now.

To properly support full precision intervals, I think we would need to add a new type to arrow that was the combination of DateTime and YearMonth -- which we would definitely not do as part of this PR :)

Again, really nice!

alamb · 2021-02-04T17:13:10Z

same comment as below about NULL vs 0 handling

👍
Yes, it's better to represent NULL as string instead of it. Thanks

alamb · 2021-02-04T17:28:50Z

💯 agree with this comment.
I think an additional restriction we should perhaps consider is 'only support integral numbers of months + years' as the YearMonthInterval interval type can not capture precisely fractional parts
https://github.com/apache/arrow/blob/master/format/Schema.fbs#L253-L254

alamb

I think this is good enough to merge. I am sure there are plenty of more improvements, but for now, this is a wonderful start. Thanks so much @ovr

@jorgecarleitao / @andygrove / @Dandandan any thoughts?

alamb · 2021-02-06T12:13:46Z

I'll plan to merge this tomorrow (Sunday) or Monday unless anyone else has any comments

alamb · 2021-02-07T11:19:10Z

Thanks again for sticking with this @ovr -- nice work and thank you

github-actionsBot added Component: Rust - DataFusion Component: Rust labels Jan 30, 2021

ovr force-pushed the issue-10816 branch from c57b70a to 1779819Compare January 30, 2021 18:38

ovr changed the title ~~ARROW-10816: [Rust][DataFushion] Initial support for Interval express…~~ ARROW-10816: [Rust][DF] Initial support for Interval expressionsJan 30, 2021

ovr marked this pull request as ready for review January 31, 2021 20:12

ovr force-pushed the issue-10816 branch from 1779819 to 9fc95aaCompare January 31, 2021 20:12

ovr changed the title ~~ARROW-10816: [Rust][DF] Initial support for Interval expressions~~ARROW-10816: [Rust][DF] Initial support for Interval expressionsJan 31, 2021

ovr force-pushed the issue-10816 branch 2 times, most recently from fc1b71b to 7cf5df7Compare January 31, 2021 20:28

ovr force-pushed the issue-10816 branch from 7cf5df7 to 30f9559Compare January 31, 2021 20:36

ovr marked this pull request as draft January 31, 2021 22:19

ovr force-pushed the issue-10816 branch 2 times, most recently from cc553e6 to f1d9d97Compare February 1, 2021 17:36

alamb reviewed Feb 1, 2021
View reviewed changes

Dandandan reviewed Feb 3, 2021
View reviewed changes

Comment threadrust/datafusion/src/sql/planner.rs Outdated

Dandandan reviewed Feb 3, 2021
View reviewed changes

ovr force-pushed the issue-10816 branch 7 times, most recently from 3717acf to 540e375Compare February 4, 2021 08:49

ovr marked this pull request as ready for review February 4, 2021 09:05

alamb reviewed Feb 4, 2021
View reviewed changes

alamb changed the title ~~ARROW-10816: [Rust][DF] Initial support for Interval expressions~~ARROW-10816: [Rust][DataFusion] Initial support for Interval expressionsFeb 4, 2021

ovr force-pushed the issue-10816 branch from 037e271 to 15f6927Compare February 5, 2021 09:51

alamb approved these changes Feb 5, 2021
View reviewed changes

Comment threadrust/datafusion/src/sql/planner.rs Outdated

ARROW-10816: [Rust][DF] Initial support for Interval expressions
6aee5ca

ovr force-pushed the issue-10816 branch from 599c3e9 to 6aee5caCompare February 6, 2021 13:11

alamb closed this in bf5714dFeb 7, 2021

asfimport mentioned this pull request May 3, 2021
[Rust] [DataFusion] Implement INTERVAL #26754
Closed

Uh oh!

Conversation

ovr commented Jan 30, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actionsBot commented Jan 30, 2021

Uh oh!

codecov-io commented Jan 30, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ovr commented Jan 31, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ovr commented Jan 31, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ovrFeb 3, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ovrFeb 4, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ovrFeb 4, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ovr commented Feb 4, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented Feb 4, 2021

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb commented Feb 6, 2021

Uh oh!

alamb commented Feb 7, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

ovr commented Jan 30, 2021•
edited
Loading

codecov-io commented Jan 30, 2021•
edited
Loading

ovr commented Jan 31, 2021•
edited
Loading

ovr commented Jan 31, 2021•
edited
Loading

ovrFeb 3, 2021•
edited
Loading

ovrFeb 4, 2021•
edited
Loading

ovrFeb 4, 2021•
edited
Loading

ovr commented Feb 4, 2021•
edited
Loading

alamb commented Feb 7, 2021•
edited
Loading