[SPARK-29387][SQL] Support `*` and `/` operators for intervals by MaxGekk · Pull Request #26132 · apache/spark

MaxGekk · 2019-10-15T18:16:32Z

What changes were proposed in this pull request?

Added new expressions MultiplyInterval and DivideInterval to multiply/divide an interval by a numeric. Updated TypeCoercion.DateTimeOperations to turn the Multiply/Divide expressions of CalendarIntervalType and NumericType to MultiplyInterval/DivideInterval.

To support new operations, added new methods multiply() and divide() to CalendarInterval.

Why are the changes needed?

To maintain feature parity with PostgreSQL which supports multiplication and division of intervals by doubles:

# select interval '1 hour' / double precision '1.5';
 ?column?
----------
 00:40:00

To conform the SQL standard which defines those operations: numeric * interval, interval * numeric and interval / numeric. See 4.5.3 Operations involving datetimes and intervals.
Improve Spark SQL UX and allow users to adjust interval columns. For example:

spark-sql> select (timestamp'now' - timestamp'yesterday') * 1.3;
interval 2 days 10 hours 39 minutes 38 seconds 568 milliseconds 900 microseconds

Does this PR introduce any user-facing change?

Yes, previously the following query fails with the error:

spark-sql> select interval 1 hour 30 minutes * 1.5;
Error in query: cannot resolve '(interval 1 hours 30 minutes * 1.5BD)' due to data type mismatch: differing types in '(interval 1 hours 30 minutes * 1.5BD)' (interval and decimal(2,1)).; line 1 pos 7;

After:

spark-sql> select interval 1 hour 30 minutes * 1.5;
interval 2 hours 15 minutes

How was this patch tested?

Added tests for the multiply() and divide() methods to CalendarIntervalSuite.java
New test suite IntervalExpressionsSuite
by tests for Multiply -> MultiplyInterval and Divide -> DivideInterval in TypeCoercionSuite
updated datetime.sql

SparkQA · 2019-10-15T20:32:52Z

Test build #112124 has finished for PR 26132 at commit 014cde5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-10-16T20:02:26Z

Test build #112182 has finished for PR 26132 at commit 049f428.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-10-16T22:27:37Z

Test build #112187 has finished for PR 26132 at commit 1ca7c89.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

MaxGekk · 2019-10-17T10:43:03Z

@wangyum @cloud-fan @srowen Please, review this PR.

srowen

It's a niche feature but looks plausible, if it's for PostgreSQL. Just want to check whether the behavior in corner cases matches.

srowen · 2019-10-17T14:26:30Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/CalendarInterval.java

  }

+  public CalendarInterval multiply(double num) {
+    int months = Math.toIntExact((long)(num * this.months));


What happens when dividing an interval of 1 month by, say, 2? You'd end up with an interval of 0 time. I suppose the right answer is whatever PostgreSQL does.

I will check what it does.

PosgreSQL does this differently. I have re-implemented the operations.

common/unsafe/src/test/java/org/apache/spark/unsafe/types/CalendarIntervalSuite.java

srowen · 2019-10-17T20:09:41Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/CalendarInterval.java

  }

+  private static CalendarInterval fromDoubles(double months, double microseconds) {
+    long roundedMonths = (long)(months);


'truncatedMonths` or something? it's not exactly rounded

common/unsafe/src/main/java/org/apache/spark/unsafe/types/CalendarInterval.java

SparkQA · 2019-10-17T22:27:58Z

Test build #112230 has finished for PR 26132 at commit 9e6745a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-10-17T22:38:32Z

Test build #112231 has finished for PR 26132 at commit b428070.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-01T17:35:37Z

Test build #113100 has finished for PR 26132 at commit 690d9c1.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-01T17:51:02Z

Test build #113107 has finished for PR 26132 at commit 5b25432.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

MaxGekk · 2019-11-01T17:59:28Z

jenkins, retest this, please

SparkQA · 2019-11-01T20:14:24Z

Test build #113109 has finished for PR 26132 at commit 5b25432.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

MaxGekk · 2019-11-01T20:27:14Z

Maybe best to wait?

@srowen @cloud-fan @dongjoon-hyun @HyukjinKwon Since #26134 has been already merged, please, take a look at this PR.

cloud-fan · 2019-11-04T09:04:34Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/CalendarInterval.java

+      double microsWithFraction) {
+    int truncatedMonths = Math.toIntExact((long)(monthsWithFraction));
+    // Using 30 days per month as PostgreSQL does.
+    double days = daysWithFraction + 30 * (monthsWithFraction - truncatedMonths);


is this consistent with pgsql? i.e. we convert the truncated months to days.

maxim=# select interval '1 month' * 1.1; ?column? -------------- 1 mon 3 days (1 row)

cloud-fan · 2019-11-04T09:25:41Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/CalendarInterval.java

+    return new CalendarInterval(truncatedMonths, truncatedDays, truncatedMicros);
+  }
+
+  public CalendarInterval multiply(double num) {


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

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.

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.

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.

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

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

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 ...

yea, precise, not big.

…l-div # Conflicts: # sql/core/src/test/resources/sql-tests/results/datetime.sql.out

SparkQA · 2019-11-04T11:55:48Z

Test build #113202 has finished for PR 26132 at commit 2265449.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class GangliaReporter extends ScheduledReporter
public static class Builder

yaooqinn · 2019-11-05T10:19:26Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala

+        MultiplyInterval(l, r)
+      case Multiply(l @ NumericType(), r @ CalendarIntervalType()) =>
+        MultiplyInterval(r, l)
+      case Divide(l @ CalendarIntervalType(), r @ NumericType()) =>


postgres=# select interval '1 year' / '365'; ?column? --------------- 23:40:16.4064 (1 row)

could this be supported?

Taking into account the discussion in #26165, I am not sure. @cloud-fan Should I support this?

We can, but this should only apply to literals, not string columns.

If we want to support it, please open another PR.

yaooqinn · 2019-11-05T10:24:27Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala

+  override def prettyName: String = operationName + "_interval"
+}
+
+case class MultiplyInterval(interval: Expression, num: Expression)


https://github.com/apache/spark/pull/26345/files#diff-b83497f7bc11578a0b63a814a2a30f48R2198-R2207
How about adding these doc and test cases here

This should be added for only expressions registered as functions.

yaooqinn · 2019-11-05T10:25:33Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala

+    (i: CalendarInterval, n: Double) => multiply(i, n),
+    "multiply")
+
+case class DivideInterval(interval: Expression, num: Expression)


https://github.com/apache/spark/pull/26345/files#diff-b83497f7bc11578a0b63a814a2a30f48R2167-R2176 ditto

SparkQA · 2019-11-05T12:43:13Z

Test build #113255 has finished for PR 26132 at commit 35ab9c0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-05T14:32:43Z

Test build #113265 has finished for PR 26132 at commit b70c0f8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-11-05T16:37:58Z

thanks, merging to master!

MaxGekk added 16 commits October 15, 2019 11:17

Add multiply to CalendarInterval

af2e7ab

Test for multiply()

4227915

Add MultiplyInterval expression

379a20a

Add divide to CalendarInterval

3e9ed0f

Test for divide()

670a7c6

Handle ArithmeticException in MultiplyInterval

3ae94cf

Test for the MultiplyInterval expression

3bce68e

Add DivideInterval expression

754109c

Test for the DivideInterval expression

166dbd8

Remove unused import

b4dc59a

Add new rules

1e2a9a6

Tests for new rules

a6b6d81

Add tests to datetime.sql

6e569c0

Regen datetime.sql.out

69a3cc7

Long -> Double

001d17b

Fix comment

014cde5

MaxGekk changed the title ~~[SPARK-29387][SQL] Support * and \ operators for intervals~~ [SPARK-29387][SQL] Support * and / operators for intervals Oct 15, 2019

Merge branch 'master' into interval-mul-div

049f428

Regen datetime.sql.out

1ca7c89

dongjoon-hyun added the SQL label Oct 16, 2019

srowen reviewed Oct 17, 2019

View reviewed changes

Implement multiply and divide as PostgreSQL does

9e6745a

srowen reviewed Oct 17, 2019

View reviewed changes

rounded -> truncated

b428070

MaxGekk added 4 commits November 1, 2019 17:52

Rebase on interval with days

d05ffa4

Regenerate datetime.sql.out

34f6605

Check round micros

00ede6c

Modify div test to check days div

690d9c1

Regenerate datetime.sql.out

5b25432

cloud-fan reviewed Nov 4, 2019

View reviewed changes

Merge remote-tracking branch 'remotes/origin/master' into interval-mu…

2265449

…l-div # Conflicts: # sql/core/src/test/resources/sql-tests/results/datetime.sql.out

maropu mentioned this pull request Nov 5, 2019

[SPARK-29688][SQL] Support average for interval type values #26347

Closed

MaxGekk added 2 commits November 5, 2019 11:43

Move multiply() and divide() to IntervalUtils

e559fb9

Use DAYS_PER_MONTH

35ab9c0

cloud-fan approved these changes Nov 5, 2019

View reviewed changes

Simplify MultiplyInterval and DivideInterval

dbc39e8

yaooqinn reviewed Nov 5, 2019

View reviewed changes

Minor

8244460

yaooqinn reviewed Nov 5, 2019

View reviewed changes

Simplify tests

b70c0f8

cloud-fan closed this in 4c53ac1 Nov 5, 2019

MaxGekk deleted the interval-mul-div branch June 5, 2020 19:41

Conversation

MaxGekk commented Oct 15, 2019

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Oct 15, 2019

Uh oh!

SparkQA commented Oct 16, 2019

Uh oh!

SparkQA commented Oct 16, 2019

Uh oh!

MaxGekk commented Oct 17, 2019

Uh oh!

srowen left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SparkQA commented Oct 17, 2019

Uh oh!

SparkQA commented Oct 17, 2019

Uh oh!

SparkQA commented Nov 1, 2019

Uh oh!

SparkQA commented Nov 1, 2019

Uh oh!

MaxGekk commented Nov 1, 2019

Uh oh!

SparkQA commented Nov 1, 2019

Uh oh!

MaxGekk commented Nov 1, 2019

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!

Choose a reason for hiding this comment

Uh oh!

yaooqinn Nov 5, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 4, 2019

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!

yaooqinn Nov 5, 2019 •

edited

Loading