[SPARK-35112][SQL] Support Cast string to day-second interval #32271

AngersZhuuuu · 2021-04-21T09:28:37Z

What changes were proposed in this pull request?

Support Cast string to day-seconds interval

Why are the changes needed?

Users can cast day-second interval string to DayTimeIntervalType.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added UT

AngersZhuuuu · 2021-04-21T09:28:45Z

FYI @MaxGekk

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

MaxGekk

Take into account days in safeFromDayTimeString()

SparkQA · 2021-04-21T11:00:39Z

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

SparkQA · 2021-04-21T11:07:40Z

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

SparkQA · 2021-04-21T15:17:29Z

Test build #137728 has finished for PR 32271 at commit b41fd25.

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

SparkQA · 2021-04-28T15:28:24Z

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

SparkQA · 2021-04-28T15:28:26Z

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

SparkQA · 2021-04-28T18:33:19Z

Test build #138046 has finished for PR 32271 at commit a82bfee.

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

AngersZhuuuu · 2021-04-29T07:39:13Z

@MaxGekk I meet a case

IntervalUtils.fromDayTimeString("2:03:04") throw exception

[info]   Cause: java.lang.IllegalArgumentException: requirement failed: Interval string must match day-time format of '^(?<sign>[+|-])?(?<day>\d+) (?<hour>\d{1,2}):(?<minute>\d{1,2}):(?<second>(\d{1,2})(\.(\d{1,9}))?)$': 2:03:04, set spark.sql.legacy.fromDayTimeString.enabled to true to restore the behavior before Spark 3.0.

It's a But right? since IntervalUtils.fromDayTimeString's comment is

  /**
   * Parse dayTime string in form: [-]d HH:mm:ss.nnnnnnnnn and [-]HH:mm:ss.nnnnnnnnn
   *
   * adapted from HiveIntervalDayTime.valueOf
   */
  def fromDayTimeString(s: String): CalendarInterval = {
    fromDayTimeString(s, DAY, SECOND)
  }

SparkQA · 2021-04-29T09:22:53Z

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

SparkQA · 2021-04-29T09:22:54Z

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

SparkQA · 2021-04-29T10:22:34Z

Kubernetes integration test unable to build dist.

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

SparkQA · 2021-04-29T13:02:16Z

Test build #138078 has finished for PR 32271 at commit 35dd9f4.

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

SparkQA · 2021-04-29T14:04:53Z

Test build #138080 has finished for PR 32271 at commit daa9675.

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

AngersZhuuuu · 2021-04-30T06:58:55Z

Current test

Incorrect evaluation (codegen off): cast(cast(INTERVAL '106751991 04:00:54.775807' DAY TO SECOND as string) as day-time interval), actual: 14454775807, expected: 9223372036854775807 (ExpressionEvalHelper.scala:209)

SparkQA · 2021-04-30T12:16:33Z

Test build #138105 has finished for PR 32271 at commit 6ff0522.

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

MaxGekk · 2021-04-30T18:12:42Z

@AngersZhuuuu Is it still WIP? Please, ping me when it is ready.

AngersZhuuuu · 2021-05-01T02:09:25Z

@MaxGekk Can be reviewed now, but currently this method does not seem to be very efficient，Maybe should I add a new method to calculate calculate total microseconds separately?

AngersZhuuuu · 2021-05-01T03:02:55Z

@MaxGekk I think it's ready for review now, but I am confused how can I use
dayTimePattern(DAY -> SECOND) in match case ?

When I write

val regex = dayTimePattern(DAY-> SECOND)

intervalStr match {
    case dayTimePattern(sign, day, hour, minute, secondAndMicro)
.......
}

It can't match, do you know how to match this? If it can be done, we can simplify more code.

SparkQA · 2021-05-01T04:25:57Z

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

SparkQA · 2021-05-01T04:25:58Z

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

SparkQA · 2021-05-01T08:22:09Z

Test build #138123 has finished for PR 32271 at commit 46fff9c.

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

SparkQA · 2021-05-01T10:23:16Z

Kubernetes integration test unable to build dist.

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

SparkQA · 2021-05-01T14:18:12Z

Test build #138127 has finished for PR 32271 at commit 50addb4.

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

MaxGekk

LGTM in general except a few comments.

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

MaxGekk · 2021-05-01T14:01:21Z

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

@@ -1775,6 +1775,48 @@ class CastSuite extends CastSuiteBase {
    }
  }

+  test("SPARK-35112: Cast string to day-time interval") {
+    checkEvaluation(cast(Literal.create("0 0:0:0"), DayTimeIntervalType), 0L)
+    checkEvaluation(cast(Literal.create("INTERVAL '0 0:0:0' DAY TO SECOND"),


Could you check INTERVAL in lower case + spaces:

Suggested change

checkEvaluation(cast(Literal.create("INTERVAL '0 0:0:0' DAY TO SECOND"),

checkEvaluation(cast(Literal.create(" interval '0 0:0:0' Day TO second "),

@AngersZhuuuu Your force push reverted the changes I approved. Please, revert them back.

@AngersZhuuuu Your force push reverted the changes I approved. Please, revert them back.

Updated

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

…essions/Cast.scala Co-authored-by: Maxim Gekk <[email protected]>

MaxGekk

LGTM, waiting for test results.

MaxGekk

Please, address comments.

SparkQA · 2021-05-01T16:27:20Z

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

SparkQA · 2021-05-01T16:27:21Z

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

SparkQA · 2021-05-01T20:13:42Z

Test build #138132 has finished for PR 32271 at commit 4f8fc78.

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

AngersZhuuuu · 2021-05-02T04:07:06Z

Please, address comments.

Sorry for my late, since yesterday's run failed so I revert and recheck, but some thing urgent break my work.

SparkQA · 2021-05-02T05:30:53Z

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

SparkQA · 2021-05-02T05:30:54Z

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

MaxGekk

+1, LGTM. Merging to master.
Thank you, @AngersZhuuuu .

SparkQA · 2021-05-02T09:12:15Z

Test build #138139 has finished for PR 32271 at commit 4e2608c.

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

cloud-fan · 2021-05-07T12:58:59Z

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

@@ -150,6 +150,58 @@ object IntervalUtils {
    }
  }

+  private val unquotedDaySecondPattern =
+    "([+|-])?(\\d+) (\\d{1,2}):(\\d{1,2}):(\\d{1,2})(\\.\\d{1,9})?"
+  private val quotedDaySecondPattern = (s"^$unquotedDaySecondPattern$$").r


unquotedDaySecondRegex?

unquotedDaySecondRegex?

Change this in #32444

AngersZhuuuu added 2 commits April 21, 2021 16:33

[SPARK-35111][SQL] Support Cast string to day-second interval

2af65de

fix

b41fd25

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

AngersZhuuuu changed the title ~~Spark 35112~~ [SPARK-35112]][SQL] Support Cast string to day-second interval Apr 21, 2021

MaxGekk reviewed Apr 21, 2021

View reviewed changes

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

MaxGekk reviewed Apr 21, 2021

View reviewed changes

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

MaxGekk requested changes Apr 21, 2021

View reviewed changes

MaxGekk changed the title ~~[SPARK-35112]][SQL] Support Cast string to day-second interval~~ [SPARK-35112][SQL] Support Cast string to day-second interval Apr 21, 2021

update

a82bfee

save

e0b3b30

save

616380e

AngersZhuuuu changed the title ~~[SPARK-35112][SQL] Support Cast string to day-second interval~~ [WIP][SPARK-35112][SQL] Support Cast string to day-second interval Apr 29, 2021

AngersZhuuuu added 2 commits April 29, 2021 16:10

update

35dd9f4

done

daa9675

update

c2c7021

update

700efa4

AngersZhuuuu changed the title ~~[WIP][SPARK-35112][SQL] Support Cast string to day-second interval~~ [SPARK-35112][SQL] Support Cast string to day-second interval May 1, 2021

AngersZhuuuu added 2 commits May 1, 2021 10:45

save

9fe4c38

Update CastSuite.scala

46fff9c

Update IntervalUtils.scala

50addb4

MaxGekk reviewed May 1, 2021

View reviewed changes

Update sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expr…

4f8fc78

…essions/Cast.scala Co-authored-by: Maxim Gekk <[email protected]>

MaxGekk approved these changes May 1, 2021

View reviewed changes

AngersZhuuuu force-pushed the SPARK-35112 branch from 2ce5af1 to 4f8fc78 Compare May 1, 2021 15:28

MaxGekk requested changes May 1, 2021

View reviewed changes

Done

4e2608c

MaxGekk approved these changes May 2, 2021

View reviewed changes

MaxGekk closed this in caa46ce May 2, 2021

cloud-fan reviewed May 7, 2021

View reviewed changes

	checkEvaluation(cast(Literal.create("INTERVAL '0 0:0:0' DAY TO SECOND"),
	checkEvaluation(cast(Literal.create(" interval '0 0:0:0' Day TO second "),

[SPARK-35112][SQL] Support Cast string to day-second interval #32271

[SPARK-35112][SQL] Support Cast string to day-second interval #32271

Conversation

AngersZhuuuu commented Apr 21, 2021 • edited by MaxGekk 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?

AngersZhuuuu commented Apr 21, 2021

MaxGekk left a comment

Choose a reason for hiding this comment

SparkQA commented Apr 21, 2021

SparkQA commented Apr 21, 2021

SparkQA commented Apr 21, 2021

SparkQA commented Apr 28, 2021

SparkQA commented Apr 28, 2021

SparkQA commented Apr 28, 2021

AngersZhuuuu commented Apr 29, 2021

SparkQA commented Apr 29, 2021

SparkQA commented Apr 29, 2021

SparkQA commented Apr 29, 2021

SparkQA commented Apr 29, 2021

SparkQA commented Apr 29, 2021

AngersZhuuuu commented Apr 30, 2021

SparkQA commented Apr 30, 2021

MaxGekk commented Apr 30, 2021

AngersZhuuuu commented May 1, 2021

AngersZhuuuu commented May 1, 2021

SparkQA commented May 1, 2021

SparkQA commented May 1, 2021

SparkQA commented May 1, 2021

SparkQA commented May 1, 2021

SparkQA commented May 1, 2021

MaxGekk left a comment

Choose a reason for hiding this comment

MaxGekk May 1, 2021

Choose a reason for hiding this comment

MaxGekk May 1, 2021

Choose a reason for hiding this comment

AngersZhuuuu May 2, 2021

Choose a reason for hiding this comment

MaxGekk left a comment

Choose a reason for hiding this comment

MaxGekk left a comment

Choose a reason for hiding this comment

SparkQA commented May 1, 2021

SparkQA commented May 1, 2021

SparkQA commented May 1, 2021

AngersZhuuuu commented May 2, 2021

SparkQA commented May 2, 2021

SparkQA commented May 2, 2021

MaxGekk left a comment

Choose a reason for hiding this comment

SparkQA commented May 2, 2021

cloud-fan May 7, 2021

Choose a reason for hiding this comment

AngersZhuuuu May 7, 2021

Choose a reason for hiding this comment

AngersZhuuuu commented Apr 21, 2021 •

edited by MaxGekk

Loading