[SPARK-35088][SQL] Accept ANSI intervals by the Sequence expression #32311

beliefer · 2021-04-23T08:55:45Z

What changes were proposed in this pull request?

This PR makes Sequence expression supports ANSI intervals as step expression.
If the start and stop expression is TimestampType, then the step expression could select year-month or day-time interval.
If the start and stop expression is DateType, then the step expression must be year-month.

Why are the changes needed?

Extends the function of Sequence expression.

Does this PR introduce any user-facing change?

'Yes'. Users could use ANSI intervals as step expression for Sequence expression.

How was this patch tested?

New tests.

SparkQA · 2021-04-23T09:15:20Z

Test build #137853 has finished for PR 32311 at commit 1e96d54.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-04-23T10:15:36Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42383/

SparkQA · 2021-04-23T10:24:18Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42388/

SparkQA · 2021-04-23T10:24:19Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42388/

SparkQA · 2021-04-23T14:44:54Z

Test build #137858 has finished for PR 32311 at commit 89a34bc.

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

MaxGekk

Could you update the comment

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

Line 2490 in 9af338c

    
                 then the step expression must resolve to the 'interval' type, otherwise to the same type

. interval relates to CalendarIntervalType

MaxGekk · 2021-04-23T14:49:22Z

...st/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala


    checkEvaluation(new Sequence(
      Literal(Timestamp.valueOf("2018-01-01 00:00:00")),
-      Literal(Timestamp.valueOf("2018-01-02 00:00:01")),


Why did you change the test for CalendarInterval? It seems it checks a specific case.

I'm dazzled.

MaxGekk · 2021-04-23T14:52:23Z

...st/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala

        Timestamp.valueOf("2018-01-01 12:00:00"),
        Timestamp.valueOf("2018-01-01 00:00:00")))

+    checkEvaluation(new Sequence(


nit: The test is already big enough. Does it make sense to put new checks to a separate test and prepend JIRA id?

MaxGekk · 2021-04-23T14:56:17Z

...st/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala

+          Literal(Date.valueOf("2018-01-01")),
+          Literal(Date.valueOf("2018-01-05")),
+          Literal(Period.ofDays(2))),
+        EmptyRow, "sequence step must be a day interval if start and end values are dates")


a day interval -> a day-time interval. Please, call DayTimeIntervalType.typeName() here. Since the types are not stable yet, we can rename the types in the near future.

MaxGekk · 2021-04-23T15:08:59Z

The error message confuses slightly:

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

Line 2567 in 9af338c

s"$prettyName only supports integral, timestamp or date types")

Type checking can fails because of unsupported type of steps even start and stop have one of: integral, timestamp or date.

MaxGekk · 2021-04-23T15:14:33Z

...st/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala

+          Literal(Date.valueOf("1970-02-01")),
+          Literal(Period.ofMonths(-1))),
+        EmptyRow,
+        s"sequence boundaries: 0 to 2678400000000 by -${28 * MICROS_PER_DAY}")


hmm, 28 because we assume 28 days per month there

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

Line 2742 in 9af338c

private val microsPerMonth = 28 * microsPerDay

?

By introducing new interval types, we tried to avoid such assumption.

Good found.
~~It seems we should change the behavior of CalenderInterval so as it could avoid such assumption.~~
~~Or the assumption looks like is a bug for the CalenderInterval.~~
~~If so , could we fix the bug in another PR?~~

In further, microsPerDay just used to estimated length of the sequences. We just need to improve the exception message so as avoid output the assume value.

MaxGekk · 2021-04-23T15:31:08Z

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


+  private class PeriodSequenceImpl[T: ClassTag]
+      (dt: IntegralType, scale: Long, fromLong: Long => T, zoneId: ZoneId)
+      (implicit num: Integral[T]) extends InternalSequenceBase(dt, scale, fromLong, zoneId) {


I am not sure it is good idea to use timestampAddInterval() in InternalSequenceBase.eval for adding months to dates. I guess DateTimeUtils.dateAddMonths() and DateTimeUtils.timestampAddInterval can return different result, especially taking into account that dateAddMonths() does not depend on the current time zone.

In fact, the current implement uses DateTimeUtils.timestampAddInterval and it's behavior seems good.

ok. Let's use timestampAddInterval since we don't have an example that could demonstrate any issues caused by timestampAddInterval().

SparkQA · 2021-04-25T08:24:39Z

Test build #137913 has finished for PR 32311 at commit 1d38d02.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-04-25T09:00:15Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42435/

SparkQA · 2021-04-25T10:03:59Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42436/

SparkQA · 2021-04-25T10:04:00Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42436/

SparkQA · 2021-04-25T10:23:28Z

Test build #137919 has finished for PR 32311 at commit cb361d1.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

MaxGekk · 2021-04-25T10:27:36Z

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

      If start and stop expressions resolve to the 'date' or 'timestamp' type
-      then the step expression must resolve to the 'interval' type, otherwise to the same type
-      as the start and stop expressions.
+      then the step expression must resolve to the 'interval' or 'year-month' or 'day-time' type,


nit:
'year-month' -> 'year-month interval'
'day-time' -> 'day-time interval'

MaxGekk · 2021-04-25T10:29:25Z

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

+           |$prettyName uses the wrong parameter type. The parameter type must conform to:
+           |1. The start and stop expressions must resolve to the same type.
+           |2. If start and stop expressions resolve to the 'date' or 'timestamp' type
+           |then the step expression must resolve to the 'interval' or 'year-month' or


Could you call YearMonthIntervalType.typeName

SparkQA · 2021-04-25T11:08:38Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42440/

SparkQA · 2021-04-25T11:08:39Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42440/

SparkQA · 2021-04-25T14:06:23Z

Test build #137914 has finished for PR 32311 at commit 75562fe.

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

SparkQA · 2021-04-26T04:30:03Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42453/

SparkQA · 2021-04-26T04:30:04Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42453/

MaxGekk

+1, LGTM. GA passed. Merging to master.
Thank you, @beliefer .

beliefer · 2021-04-26T07:14:05Z

@MaxGekk Thanks for you review.

SparkQA · 2021-04-26T07:56:36Z

Test build #137932 has finished for PR 32311 at commit 47853e8.

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

AmplabJenkins · 2021-04-26T08:23:50Z

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

Accept ANSI intervals by the Sequence expression

1e96d54

github-actions bot added the SQL label Apr 23, 2021

beliefer changed the title ~~Accept ANSI intervals by the Sequence expression~~ [SPARK-35088][SQL] Accept ANSI intervals by the Sequence expression Apr 23, 2021

Fix scala style

89a34bc

MaxGekk reviewed Apr 23, 2021

View reviewed changes

Update code

1d38d02

Fix scala style

75562fe

Improve the error msg.

cb361d1

MaxGekk reviewed Apr 25, 2021

View reviewed changes

Update code

47853e8

MaxGekk approved these changes Apr 26, 2021

View reviewed changes

MaxGekk closed this in c0a3c0c Apr 26, 2021

[SPARK-35088][SQL] Accept ANSI intervals by the Sequence expression #32311

[SPARK-35088][SQL] Accept ANSI intervals by the Sequence expression #32311

Uh oh!

Conversation

beliefer commented Apr 23, 2021

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 Apr 23, 2021

Uh oh!

SparkQA commented Apr 23, 2021

Uh oh!

SparkQA commented Apr 23, 2021

Uh oh!

SparkQA commented Apr 23, 2021

Uh oh!

SparkQA commented Apr 23, 2021

Uh oh!

MaxGekk 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!

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Apr 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beliefer Apr 25, 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!

beliefer Apr 25, 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!

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 Apr 25, 2021

Uh oh!

SparkQA commented Apr 25, 2021

Uh oh!

SparkQA commented Apr 25, 2021

Uh oh!

SparkQA commented Apr 25, 2021

Uh oh!

SparkQA commented Apr 25, 2021

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!

SparkQA commented Apr 25, 2021

Uh oh!

SparkQA commented Apr 25, 2021

Uh oh!

SparkQA commented Apr 25, 2021

Uh oh!

SparkQA commented Apr 26, 2021

Uh oh!

SparkQA commented Apr 26, 2021

Uh oh!

MaxGekk left a comment

beliefer Apr 25, 2021 •

edited

Loading

beliefer Apr 25, 2021 •

edited

Loading