[SPARK-34761][SQL] Support add/subtract of a day-time interval to/from a timestamp #31855

MaxGekk · 2021-03-16T17:29:16Z

What changes were proposed in this pull request?

Support timestamp +/- day-time interval. In the PR, I propose to extend the TimeAdd expression and support DayTimeIntervalType as the interval parameter. The expression invokes the new method DateTimeUtils.timestampAddDayTime() which splits the input day-time interval to days and microsecond adjustment of a day, and adds days (and the microseconds) to a local timestamp derived from the given timestamp at the given time zone. The resulted local timestamp is converted back to the offset in microseconds since the epoch.

Also I updated the rules that handle CalendarIntervalType and produce TimeAdd to take into account new type DateTimeIntervalType for the interval parameter of TimeAdd.

Why are the changes needed?

To conform the ANSI SQL standard which requires to support such operation over timestamps and intervals:

Does this PR introduce any user-facing change?

Should not since new intervals have not been released yet.

How was this patch tested?

By running new tests:

$ build/sbt "test:testOnly *DateTimeUtilsSuite"
$ build/sbt "test:testOnly *DateExpressionsSuite"
$ build/sbt "test:testOnly *ColumnExpressionSuite"

AmplabJenkins · 2021-03-16T17:58:53Z

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136124/

AmplabJenkins · 2021-03-16T18:00:36Z

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40706/

AmplabJenkins · 2021-03-16T22:44:12Z

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40711/

AmplabJenkins · 2021-03-16T22:44:12Z

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136129/

MaxGekk · 2021-03-17T04:46:09Z

@cloud-fan @yaooqinn Could you review this PR, please.

MaxGekk · 2021-03-17T05:33:22Z

@cloud-fan @srielau I doubt about semantic of the operation. Should we add the interval as "physical" duration like in this PR by just shifting timestamp offset in micros, or convert the timestamp to a local timestamp and add days and times?

cloud-fan · 2021-03-17T06:21:30Z

I think we should add days and times separately, to be consistent with the legacy interval type which has days as a field.

yaooqinn · 2021-03-17T07:08:20Z

I think we should add days and times separately, to be consistent with the legacy interval type which has days as a field.

+1 for this suggestion

…ime-interval

cloud-fan · 2021-03-18T16:45:00Z

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

@@ -342,11 +342,12 @@ class Analyzer(override val catalogManager: CatalogManager)
          case (YearMonthIntervalType, DateType) => DateAddYMInterval(r, l)
          case (TimestampType, YearMonthIntervalType) => TimestampAddYMInterval(l, r)
          case (YearMonthIntervalType, TimestampType) => TimestampAddYMInterval(r, l)
-          case (CalendarIntervalType, CalendarIntervalType) => a
+          case (CalendarIntervalType, CalendarIntervalType) |
+               (DayTimeIntervalType, DayTimeIntervalType) => a


shall we allow adding year-month interval as well?

The op is allowed already, see the test https://github.com/apache/spark/pull/31789/files#diff-bff10c8a3182aa943d8927135e0c14b02a338d9bcca94ddcd77670ee01fea0f3R2380 . It should still pass after the changes.

I have to add this cases because I reuse the TimeAdd expression, and associated rules here.

The year-month interval +/- year-month interval expr is handled by the default case case _ => a/case _ => s.

cloud-fan · 2021-03-18T16:45:41Z

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

@@ -356,10 +357,11 @@ class Analyzer(override val catalogManager: CatalogManager)
            DatetimeSub(l, r, DateAddYMInterval(l, UnaryMinus(r, f)))
          case (TimestampType, YearMonthIntervalType) =>
            DatetimeSub(l, r, TimestampAddYMInterval(l, UnaryMinus(r, f)))
-          case (CalendarIntervalType, CalendarIntervalType) => s
+          case (CalendarIntervalType, CalendarIntervalType) |
+               (DayTimeIntervalType, DayTimeIntervalType) => s


The YearMonthIntervalType is handled by the default case case _ => s. We need to handle CalendarIntervalType and DayTimeIntervalType especially otherwise they will be casted to some unexpected type in the base arithmetic ops +/-

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala

AmplabJenkins · 2021-03-18T17:07:47Z

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136216/

AmplabJenkins · 2021-03-18T17:09:32Z

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40798/

MaxGekk · 2021-03-18T17:17:58Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/LiteralGenerator.scala

-      nanoAdjustment <- Gen.choose(-999999000, 999999000)
-    } yield {
-      Literal.create(Duration.ofSeconds(seconds, nanoAdjustment), DayTimeIntervalType)
+    calendarIntervalLiterGen.map { calendarIntervalLiteral =>


This generator is used to test expressions like random timestamp + random day-time interval. I have to adjust the generator to have reasonable intervals, and prevent overflows.

AmplabJenkins · 2021-03-18T18:07:18Z

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136219/

AmplabJenkins · 2021-03-18T18:09:03Z

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40801/

cloud-fan · 2021-03-19T04:01:17Z

thanks, merging to master!

MaxGekk added 4 commits March 16, 2021 14:03

Add TimestampAddDTInterval

e4b299d

Add test for the TimestampAddDTInterval expression

90c46af

Support +/-

92a855b

Fix imports

1868930

Trigger build

a14e90a

github-actions bot added the SQL label Mar 16, 2021

MaxGekk added 5 commits March 18, 2021 16:01

Merge remote-tracking branch 'origin/master' into timestamp-add-day-t…

6cce729

…ime-interval

Add days and micros

5833bd0

Add comments for timestampAddDayTime()

c861a17

Add a test for timestampAddDayTime()

84af890

Fix rules

66874b6

cloud-fan reviewed Mar 18, 2021

View reviewed changes

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala Show resolved Hide resolved

cloud-fan approved these changes Mar 18, 2021

View reviewed changes

Add more tests

c851449

MaxGekk commented Mar 18, 2021

View reviewed changes

cloud-fan approved these changes Mar 19, 2021

View reviewed changes

cloud-fan closed this in a48b208 Mar 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-34761][SQL] Support add/subtract of a day-time interval to/from a timestamp #31855

[SPARK-34761][SQL] Support add/subtract of a day-time interval to/from a timestamp #31855

MaxGekk commented Mar 16, 2021 •

edited

Loading

AmplabJenkins commented Mar 16, 2021

AmplabJenkins commented Mar 16, 2021

AmplabJenkins commented Mar 16, 2021

AmplabJenkins commented Mar 16, 2021

MaxGekk commented Mar 17, 2021

MaxGekk commented Mar 17, 2021

cloud-fan commented Mar 17, 2021

yaooqinn commented Mar 17, 2021

cloud-fan Mar 18, 2021

MaxGekk Mar 18, 2021

cloud-fan Mar 18, 2021

MaxGekk Mar 18, 2021

AmplabJenkins commented Mar 18, 2021

AmplabJenkins commented Mar 18, 2021

MaxGekk Mar 18, 2021

AmplabJenkins commented Mar 18, 2021

AmplabJenkins commented Mar 18, 2021

cloud-fan commented Mar 19, 2021

[SPARK-34761][SQL] Support add/subtract of a day-time interval to/from a timestamp #31855

[SPARK-34761][SQL] Support add/subtract of a day-time interval to/from a timestamp #31855

Conversation

MaxGekk commented Mar 16, 2021 • edited Loading

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?

AmplabJenkins commented Mar 16, 2021

AmplabJenkins commented Mar 16, 2021

AmplabJenkins commented Mar 16, 2021

AmplabJenkins commented Mar 16, 2021

MaxGekk commented Mar 17, 2021

MaxGekk commented Mar 17, 2021

cloud-fan commented Mar 17, 2021

yaooqinn commented Mar 17, 2021

cloud-fan Mar 18, 2021

Choose a reason for hiding this comment

MaxGekk Mar 18, 2021

Choose a reason for hiding this comment

cloud-fan Mar 18, 2021

Choose a reason for hiding this comment

MaxGekk Mar 18, 2021

Choose a reason for hiding this comment

AmplabJenkins commented Mar 18, 2021

AmplabJenkins commented Mar 18, 2021

MaxGekk Mar 18, 2021

Choose a reason for hiding this comment

AmplabJenkins commented Mar 18, 2021

AmplabJenkins commented Mar 18, 2021

cloud-fan commented Mar 19, 2021

MaxGekk commented Mar 16, 2021 •

edited

Loading