[SPARK-31527][SQL] date add/subtract interval only allow those day precision in ansi mode #28310

yaooqinn · 2020-04-23T08:22:33Z

What changes were proposed in this pull request?

To follow ANSI，the expressions - date + interval, interval + date and date - interval should only accept intervals which the microseconds part is 0.

Why are the changes needed?

Better ANSI compliance

Does this PR introduce any user-facing change?

No, this PR should target 3.0.0 in which this feature is newly added.

How was this patch tested?

add more unit tests

…ecision in ansi mode

yaooqinn · 2020-04-23T08:25:06Z

cc: @cloud-fan @dongjoon-hyun @HyukjinKwon many thanks.

SparkQA · 2020-04-23T08:28:06Z

Test build #121666 has started for PR 28310 at commit 77e75a7.

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

SparkQA · 2020-04-23T19:18:22Z

Test build #121681 has finished for PR 28310 at commit 9d7ea7e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class DateAddInterval(

cloud-fan · 2020-04-24T03:09:03Z

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

        }
        case s @ Subtract(l, r) if s.childrenResolved => (l.dataType, r.dataType) match {
          case (CalendarIntervalType, CalendarIntervalType) => s
+          case (DateType, CalendarIntervalType) => DateAddInterval(l, UnaryMinus(r))


This is a good idea, maybe we can remove TimeSub later.

yea, we can clean it up later

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala

SparkQA · 2020-04-24T06:53:45Z

Test build #121708 has finished for PR 28310 at commit 0c96bf5.

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

SparkQA · 2020-04-24T07:05:02Z

Test build #121720 has finished for PR 28310 at commit 65a0e4d.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2020-04-24T07:33:03Z

retest this please

SparkQA · 2020-04-24T13:23:52Z

Test build #121738 has finished for PR 28310 at commit 65a0e4d.

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

cloud-fan · 2020-04-24T13:32:58Z

seems legitimate test failures

yaooqinn · 2020-04-24T13:34:49Z

My mistake. Forgot to re-generate SQL stript test results

SparkQA · 2020-04-24T19:28:43Z

Test build #121765 has finished for PR 28310 at commit c1964a8.

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

SparkQA · 2020-04-25T10:19:31Z

Test build #121799 has finished for PR 28310 at commit c64d27e.

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

yaooqinn · 2020-04-25T11:37:27Z

retest this please

yaooqinn · 2020-04-25T11:44:44Z

The test failure is unrelated

the org.apache.spark.sql.execution.ui.AllExecutionsPageSuite.SPARK-27019:correctly display SQL page when event reordering happens is flaky for just checking the html not containing 1970. I will add a ticket to check and fix that.
In the specific failure above, it failed because

...
<td sorttable_customkey="1587806019707">
...

contained 1970

SparkQA · 2020-04-25T12:12:55Z

Test build #121804 has finished for PR 28310 at commit c64d27e.

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

yaooqinn · 2020-04-25T12:32:58Z

retest this please

SparkQA · 2020-04-25T17:48:31Z

Test build #121805 has finished for PR 28310 at commit c64d27e.

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

cloud-fan · 2020-04-27T05:28:40Z

thanks, merging to master/3.0!

…ecision in ansi mode To follow ANSI，the expressions - `date + interval`, `interval + date` and `date - interval` should only accept intervals which the `microseconds` part is 0. Better ANSI compliance No, this PR should target 3.0.0 in which this feature is newly added. add more unit tests Closes #28310 from yaooqinn/SPARK-31527. Authored-by: Kent Yao <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit ebc8fa5) Signed-off-by: Wenchen Fan <[email protected]>

### What changes were proposed in this pull request? The implementation of TimeSub for the operation of timestamp subtracting interval is almost repetitive with TimeAdd. We can replace it with TimeAdd(l, -r) since there are equivalent. Suggestion from #28310 (comment) Besides, the Coercion rules for TimeAdd/TimeSub(date, interval) are useless anymore, so remove them in this PR since they are touched in this PR. ### Why are the changes needed? remove redundant and useless code for easy maintenance ### Does this PR introduce any user-facing change? Yes, the SQL string of `datetime - interval` become `datetime + (- interval)` ### How was this patch tested? modified existing unit tests. Closes #28381 from yaooqinn/SPARK-31586. Authored-by: Kent Yao <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

… add/subtract interval operations ### What changes were proposed in this pull request? With #28310, the operation of date +/- interval(m, d, 0) has been improved a lot. According to the benchmark results, about 75% time cost is reduced because of no casting date to timestamp back and forth. In this PR, we add a benchmark for these operations, and timestamp +/- interval operations as accessories. ### Why are the changes needed? Performance test coverage, since these operations are missing in the DateTimeBenchmark. ### Does this PR introduce any user-facing change? No, just test ### How was this patch tested? regenerated benchmark results Closes #28369 from yaooqinn/SPARK-31527-F. Authored-by: Kent Yao <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

… add/subtract interval operations ### What changes were proposed in this pull request? With #28310, the operation of date +/- interval(m, d, 0) has been improved a lot. According to the benchmark results, about 75% time cost is reduced because of no casting date to timestamp back and forth. In this PR, we add a benchmark for these operations, and timestamp +/- interval operations as accessories. ### Why are the changes needed? Performance test coverage, since these operations are missing in the DateTimeBenchmark. ### Does this PR introduce any user-facing change? No, just test ### How was this patch tested? regenerated benchmark results Closes #28369 from yaooqinn/SPARK-31527-F. Authored-by: Kent Yao <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 54996be) Signed-off-by: Wenchen Fan <[email protected]>

MaxGekk · 2020-05-03T18:43:06Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala

+     interval: CalendarInterval): SQLDate = {
+    require(interval.microseconds == 0,
+      "Cannot add hours, minutes or seconds, milliseconds, microseconds to a date")
+    val ld = LocalDate.ofEpochDay(start).plusMonths(interval.months).plusDays(interval.days)


Actually, the result depends on the order of plusMonths() and plusDays(). @yaooqinn Did you make the choice intentionally? I am asking you because adding days and months can be much cheaper.

Yes，here we are follow the previous behavior of using timestamp + interval

I see, thanks. It would be nice to document such behavior of this function and timestampAddInterval somewhere. It is not obvious that we add month then days and then micros. The order could be opposite.

FYI, in snowflake internal '1 month 1 day' is different from internal '1 day 1 month'. We should at least document our own behavior.

I will make time for that PR.

[SPARK-31527][SQL] date add/subtract interval only allow those day pr…

77e75a7

…ecision in ansi mode

probot-autolabeler bot added the SQL label Apr 23, 2020

cloud-fan reviewed Apr 23, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated Show resolved Hide resolved

address comments

9d7ea7e

fix tests

0c96bf5

cloud-fan reviewed Apr 24, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Apr 24, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated Show resolved Hide resolved

address comment

65a0e4d

fix test

c1964a8

Merge branch 'master' into SPARK-31527

c64d27e

yaooqinn requested a review from cloud-fan April 27, 2020 05:24

cloud-fan closed this in ebc8fa5 Apr 27, 2020

This was referenced Apr 27, 2020

[SPARK-31527][SQL][TESTS][FOLLOWUP] Add a benchmark test for datetime add/subtract interval operations #28369

Closed

[SPARK-31586][SQL] Replace expression TimeSub(l, r) with TimeAdd(l -r) #28381

Closed

MaxGekk reviewed May 3, 2020

View reviewed changes

[SPARK-31527][SQL] date add/subtract interval only allow those day precision in ansi mode #28310

[SPARK-31527][SQL] date add/subtract interval only allow those day precision in ansi mode #28310

Uh oh!

Conversation

yaooqinn commented Apr 23, 2020

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!

yaooqinn commented Apr 23, 2020

Uh oh!

SparkQA commented Apr 23, 2020

Uh oh!

Uh oh!

SparkQA commented Apr 23, 2020

Uh oh!

cloud-fan Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

yaooqinn Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Apr 24, 2020

Uh oh!

SparkQA commented Apr 24, 2020

Uh oh!

yaooqinn commented Apr 24, 2020

Uh oh!

SparkQA commented Apr 24, 2020

Uh oh!

cloud-fan commented Apr 24, 2020

Uh oh!

yaooqinn commented Apr 24, 2020

Uh oh!

SparkQA commented Apr 24, 2020

Uh oh!

SparkQA commented Apr 25, 2020

Uh oh!

yaooqinn commented Apr 25, 2020

Uh oh!

yaooqinn commented Apr 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Apr 25, 2020

Uh oh!

yaooqinn commented Apr 25, 2020

Uh oh!

SparkQA commented Apr 25, 2020

Uh oh!

cloud-fan commented Apr 27, 2020

Uh oh!

MaxGekk May 3, 2020

Choose a reason for hiding this comment

Uh oh!

yaooqinn May 3, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk May 3, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan May 5, 2020

Choose a reason for hiding this comment

Uh oh!

yaooqinn May 6, 2020 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yaooqinn commented Apr 25, 2020 •

edited

Loading

yaooqinn May 6, 2020 •

edited

Loading