Skip to content

[SPARK-29387][SQL] Support * and / operators for intervals #26132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
MaxGekk wants to merge 36 commits into from
Closed

[SPARK-29387][SQL] Support * and / operators for intervals #26132

Changes from 1 commit
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
af2e7ab
Add multiply to CalendarInterval
MaxGekk Oct 15, 2019
4227915
Test for multiply()
MaxGekk Oct 15, 2019
379a20a
Add MultiplyInterval expression
MaxGekk Oct 15, 2019
3e9ed0f
Add divide to CalendarInterval
MaxGekk Oct 15, 2019
670a7c6
Test for divide()
MaxGekk Oct 15, 2019
3ae94cf
Handle ArithmeticException in MultiplyInterval
MaxGekk Oct 15, 2019
3bce68e
Test for the MultiplyInterval expression
MaxGekk Oct 15, 2019
754109c
Add DivideInterval expression
MaxGekk Oct 15, 2019
166dbd8
Test for the DivideInterval expression
MaxGekk Oct 15, 2019
b4dc59a
Remove unused import
MaxGekk Oct 15, 2019
1e2a9a6
Add new rules
MaxGekk Oct 15, 2019
a6b6d81
Tests for new rules
MaxGekk Oct 15, 2019
6e569c0
Add tests to datetime.sql
MaxGekk Oct 15, 2019
69a3cc7
Regen datetime.sql.out
MaxGekk Oct 15, 2019
001d17b
Long -> Double
MaxGekk Oct 15, 2019
014cde5
Fix comment
MaxGekk Oct 15, 2019
049f428
Merge branch 'master' into interval-mul-div
MaxGekk Oct 16, 2019
1ca7c89
Regen datetime.sql.out
MaxGekk Oct 16, 2019
9e6745a
Implement multiply and divide as PostgreSQL does
MaxGekk Oct 17, 2019
b428070
rounded -> truncated
MaxGekk Oct 17, 2019
8ad4001
Merge remote-tracking branch 'origin/master' into interval-mul-div
MaxGekk Oct 18, 2019
91337e5
Merge remote-tracking branch 'remotes/origin/master' into interval-mu…
MaxGekk Oct 25, 2019
2bb916f
Add new line at the end of datetime.sql
MaxGekk Oct 25, 2019
6ba53f0
Avoid fromString() in CalendarIntervalSuite
MaxGekk Oct 25, 2019
719fe6c
Merge remote-tracking branch 'remotes/origin/master' into interval-mu…
MaxGekk Nov 1, 2019
d05ffa4
Rebase on interval with days
MaxGekk Nov 1, 2019
34f6605
Regenerate datetime.sql.out
MaxGekk Nov 1, 2019
00ede6c
Check round micros
MaxGekk Nov 1, 2019
690d9c1
Modify div test to check days div
MaxGekk Nov 1, 2019
5b25432
Regenerate datetime.sql.out
MaxGekk Nov 1, 2019
2265449
Merge remote-tracking branch 'remotes/origin/master' into interval-mu…
MaxGekk Nov 4, 2019
e559fb9
Move multiply() and divide() to IntervalUtils
MaxGekk Nov 5, 2019
35ab9c0
Use DAYS_PER_MONTH
MaxGekk Nov 5, 2019
dbc39e8
Simplify MultiplyInterval and DivideInterval
MaxGekk Nov 5, 2019
8244460
Minor
MaxGekk Nov 5, 2019
b70c0f8
Simplify tests
MaxGekk Nov 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
rounded -> truncated
  • Loading branch information
@MaxGekk
MaxGekk committed Oct 17, 2019
commit b4280705cb98a5e03529b638b2c1cf4213db6197
8 changes: 4 additions & 4 deletions common/unsafe/src/main/java/org/apache/spark/unsafe/types/CalendarInterval.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,11 +356,11 @@ public CalendarInterval subtract(CalendarInterval that) {
}

private static CalendarInterval fromDoubles(double months, double microseconds) {
long roundedMonths = (long)(months);
long truncatedMonths = (long)(months);
// Using 30 days per month as PostgreSQL does.
double microsInMonth = 30 * MICROS_PER_DAY * (months - roundedMonths);
long roundedMicroseconds = (long)(microseconds + microsInMonth);
return new CalendarInterval(Math.toIntExact(roundedMonths), roundedMicroseconds);
double microsInMonth = 30 * MICROS_PER_DAY * (months - truncatedMonths);
long truncatedMicroseconds = (long)(microseconds + microsInMonth);
return new CalendarInterval(Math.toIntExact(truncatedMonths), truncatedMicroseconds);
}

public CalendarInterval multiply(double num) {
Copy link
Contributor

@cloud-fan cloud-fan Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we use decimal instead? double is approximate value and we may truncate.

Copy link
Member Author

@MaxGekk MaxGekk Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • In that case, our implementation will deviate from postgesql which uses double internally. At the moment, we return the same results as postgresql (or I haven't found yet the case when the results are different).
  • most likely, it will be slower

If it is ok, I will switch to decimals.

Copy link
Contributor

@cloud-fan cloud-fan Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK let's use double if it's what pgsql uses.

Can we move the add, subtract, multiply and divide to IntervalUtils? In case we want to change them in the future.

Copy link
Member Author

@MaxGekk MaxGekk Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move the add, subtract ... to IntervalUtils?

Do you want to move + and - in this PR? I just want to double check this because those methods are not related to this PR directly.

Copy link
Contributor

@cloud-fan cloud-fan Nov 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both are fine. We can move them tother, or have a followup PR to move +/-

Copy link
Member

@yaooqinn yaooqinn Nov 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double is not big enough to support the average aggregate for interval #26347, I prefer decimal personally

Copy link
Member Author

@MaxGekk MaxGekk Nov 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double is not big enough to support the average aggregate ...

@yaooqinn Could you explain what do you mean? I could image that double is not precise enough but not big though ...

Copy link
Member

@yaooqinn yaooqinn Nov 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, precise, not big.

Expand Down