[SPARK-35111][SQL] Support Cast string to year-month interval #32266

AngersZhuuuu · 2021-04-21T08:20:56Z

What changes were proposed in this pull request?

Support Cast string to year-month interval
Supported format as below

ANSI_STYLE, like
INTERVAL -'-10-1' YEAR TO MONTH
HIVE_STYLE like
10-1 or -10-1


Rules from the SQL standard about ANSI_STYLE:

<interval literal> ::=
  INTERVAL [ <sign> ] <interval string> <interval qualifier>
<interval string> ::=
  <quote> <unquoted interval string> <quote>
<unquoted interval string> ::=
  [ <sign> ] { <year-month literal> | <day-time literal> }
<year-month literal> ::=
  <years value> [ <minus sign> <months value> ]
  | <months value>
<years value> ::=
  <datetime value>
<months value> ::=
  <datetime value>
<datetime value> ::=
  <unsigned integer>
<unsigned integer> ::= <digit>...

Why are the changes needed?

Support Cast string to year-month interval

Does this PR introduce any user-facing change?

User can cast year month interval string to YearMonthIntervalType

How was this patch tested?

Added UT

AngersZhuuuu · 2021-04-21T08:21:12Z

FYI @MaxGekk

SparkQA · 2021-04-21T08:26:50Z

Test build #137721 has finished for PR 32266 at commit 9546a0f.

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

MaxGekk · 2021-04-21T08:36:21Z

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

@@ -1774,6 +1774,19 @@ class CastSuite extends CastSuiteBase {
      assert(e3.contains("Casting 2147483648 to int causes overflow"))
    }
  }
+
+  test("SPARK-35111: Cast string to year-month interval") {


Could you add round trip tests: string -> year-month interval -> string, and year-month interval -> string -> year-month interval

Could you test corner cases when arithmetic overflow happens. Also test upper and lower cases.

Could you test corner cases when arithmetic overflow happens. Also test upper and lower cases.

Current test is not correct according to #32281, will update after #32281 merged

Please, check the input in lower case. For example:

checkEvaluation(cast(Literal.create(" interval '1-0' YEAR TO MONTH "), YearMonthIntervalType), 12)

MaxGekk · 2021-04-21T08:37:31Z

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

+      if (input == null || input.toString == null) {
+        throw new IllegalArgumentException("Interval year-month string must be not null")
+      } else {
+        val regex = "INTERVAL '([-|+]?[0-9]+-[-|+]?[0-9]+)' YEAR TO MONTH".r


The keywords INTERVAL and YEAR TO MONTH are not mandatory by SQL standard. We should be able to parse only payload too.

Also could you move the regexp out of the method. So, we can avoid its "compilation" per every call.

Regarding to the pattern '([-|+]?[0-9]+-[-|+]?[0-9]+)', should we handle the second sign before month? Take a look at the val above yearMonthPattern

Regarding to the pattern '([-|+]?[0-9]+-[-|+]?[0-9]+)', should we handle the second sign before month? Take a look at the val above yearMonthPattern

How about current

@MaxGekk Can you help to list the case we need to support here,

INTERVAL [-|+]'[+|-]0-0' YEAR TO MONTH [+|-]0-0

Any more case need to support here?

First of all, we must support our own output (in case insensitive manner by default but ideally we should respect the SQL config spark.sql.caseSensitive):

ANSI_STYLE, like
INTERVAL -'-10-1' YEAR TO MONTH

HIVE_STYLE like
10-1 or -10-1

Rules from the SQL standard:

<interval literal> ::= INTERVAL [ <sign> ] <interval string> <interval qualifier> <interval string> ::= <quote> <unquoted interval string> <quote> <unquoted interval string> ::= [ <sign> ] { <year-month literal> | <day-time literal> } <year-month literal> ::= <years value> [ <minus sign> <months value> ] | <months value> <years value> ::= <datetime value> <months value> ::= <datetime value> <datetime value> ::= <unsigned integer> <unsigned integer> ::= <digit>...

but ideally we should respect the SQL config spark.sql.caseSensitive):

Spark Catalyst parser not respect spark.sql.caseSensitive too, should we respect it here?

For parsing I think it's OK to be always case insensitive. The Spark SQL parser is case insensitive.

Ok, how about current approach

SparkQA · 2021-04-21T09:31:59Z

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

SparkQA · 2021-04-21T09:32:01Z

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

SparkQA · 2021-04-21T10:01:10Z

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

SparkQA · 2021-04-21T10:01:11Z

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

SparkQA · 2021-04-21T13:31:02Z

Test build #137725 has finished for PR 32266 at commit 691c1f4.

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

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

SparkQA · 2021-04-22T09:39:25Z

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

SparkQA · 2021-04-22T09:39:26Z

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

SparkQA · 2021-04-22T12:23:50Z

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

SparkQA · 2021-04-22T12:23:51Z

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

SparkQA · 2021-04-22T13:23:11Z

Test build #137791 has finished for PR 32266 at commit 62d175b.

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

SparkQA · 2021-04-22T16:05:57Z

Test build #137803 has finished for PR 32266 at commit 6d14414.

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

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

cloud-fan · 2021-04-22T17:11:07Z

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

@@ -93,6 +93,23 @@ object IntervalUtils {

  private val yearMonthPattern = "^([+|-])?(\\d+)-(\\d+)$".r

+  private val yearMonthFuzzyPattern = "[^-|+]*?([+|-]?[\\d]+-[\\d]+).*".r


why do we need a new regex?

why do we need a new regex?

Since yearMonthPattern only support [+|-]yy-mmmm

what do we want to support here?

I am discussing this here #32266 (comment) with @MaxGekk ,
Do you have any other supplementary suggestions?

SparkQA · 2021-04-23T14:01:23Z

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

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

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

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

SparkQA · 2021-04-29T07:31:40Z

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

SparkQA · 2021-04-29T07:31:41Z

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

SparkQA · 2021-04-29T07:33:28Z

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

SparkQA · 2021-04-29T08:23:06Z

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

SparkQA · 2021-04-29T08:23:07Z

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

SparkQA · 2021-04-29T08:39:42Z

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

SparkQA · 2021-04-29T08:45:37Z

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

SparkQA · 2021-04-29T09:20:46Z

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

SparkQA · 2021-04-29T09:20:47Z

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

SparkQA · 2021-04-29T11:06:39Z

Test build #138072 has finished for PR 32266 at commit ca19c09.

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

SparkQA · 2021-04-29T11:35:14Z

Test build #138073 has finished for PR 32266 at commit 3adde87.

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

SparkQA · 2021-04-29T12:10:20Z

Test build #138075 has finished for PR 32266 at commit 2c8785b.

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

SparkQA · 2021-04-29T12:16:06Z

Test build #138076 has finished for PR 32266 at commit 253c70e.

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

SparkQA · 2021-04-29T13:32:42Z

Test build #138079 has finished for PR 32266 at commit 9c70b88.

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

MaxGekk · 2021-04-29T16:56:34Z

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

+    "^(INTERVAL\\s+)([+|-])?(')([+|-])?(\\d+)-(\\d+)(')(\\s+YEAR\\s+TO\\s+MONTH)$".r
+
+  def castStringToYMInterval(input: UTF8String): Int = {
+    input.trimAll().toString.toUpperCase(Locale.ROOT) match {


You can specify that your pattern is case insensitive, and remove .toUpperCase(Locale.ROOT)

You don't see any tests that check proper spaces handling - I mean trimAll(). Could you add such checks.

Just wonder why don't you trim whitespaces by regular expression?

regular

Match case -1-0

You don't see any tests that check proper spaces handling - I mean trimAll(). Could you add such checks.

done

MaxGekk · 2021-04-29T16:57:22Z

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

@@ -92,6 +93,25 @@ object IntervalUtils {
  }

  private val yearMonthPattern = "^([+|-])?(\\d+)-(\\d+)$".r
+  private val yearMonthStringPattern =
+    "^(INTERVAL\\s+)([+|-])?(')([+|-])?(\\d+)-(\\d+)(')(\\s+YEAR\\s+TO\\s+MONTH)$".r


Suggested change

"^(INTERVAL\\s+)([+|-])?(')([+|-])?(\\d+)-(\\d+)(')(\\s+YEAR\\s+TO\\s+MONTH)$".r

"(?i)^(INTERVAL\\s+)([+|-])?(')([+|-])?(\\d+)-(\\d+)(')(\\s+YEAR\\s+TO\\s+MONTH)$".r

The (?i) is placed at the beginning of the pattern to enable case-insensitivity.

Thanks for you share

MaxGekk · 2021-04-29T17:04:19Z

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

+  private[this] def castToYearMonthIntervalCode(from: DataType): CastFunction = from match {
+    case StringType =>
+      val util = IntervalUtils.getClass.getCanonicalName.stripSuffix("$")
+      (c, evPrim, evNull) => code"$evPrim = $util.castStringToYMInterval($c);"


nit: evNull is not used

Suggested change

(c, evPrim, evNull) => code"$evPrim = $util.castStringToYMInterval($c);"

(c, evPrim, _) => code"$evPrim = $util.castStringToYMInterval($c);"

MaxGekk

LGTM in general except of a few comments.

SparkQA · 2021-04-30T03:30:41Z

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

SparkQA · 2021-04-30T03:35:06Z

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

MaxGekk · 2021-04-30T05:02:23Z

+1, LGTM. GA passed. Merging to master.
Thank you @AngersZhuuuu , and @cloud-fan for review.

SparkQA · 2021-04-30T07:16:47Z

Test build #138096 has finished for PR 32266 at commit 5ca83ab.

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

[SPARK-35111][SQL] Support Cast string to year-month interval

9546a0f

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

Update CastSuite.scala

691c1f4

MaxGekk reviewed Apr 21, 2021

View reviewed changes

AngersZhuuuu added 4 commits April 22, 2021 08:39

Update CastSuite.scala

f5b02ee

Update IntervalUtils.scala

15424a7

Update CastSuite.scala

879817b

Update CastSuite.scala

62d175b

cloud-fan reviewed Apr 22, 2021

View reviewed changes

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

AngersZhuuuu added 3 commits April 22, 2021 18:12

save

2c75bba

Merge branch 'master' into SPARK-SPARK-35111

5b134fa

Update CastSuite.scala

6d14414

cloud-fan reviewed Apr 22, 2021

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Apr 22, 2021

View reviewed changes

Update Cast.scala

d19bbc8

cloud-fan reviewed Apr 29, 2021

View reviewed changes

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

cloud-fan reviewed Apr 29, 2021

View reviewed changes

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

AngersZhuuuu added 2 commits April 29, 2021 15:41

follow comment

253c70e

Update IntervalUtils.scala

9c70b88

cloud-fan approved these changes Apr 29, 2021

View reviewed changes

MaxGekk reviewed Apr 29, 2021

View reviewed changes

MaxGekk approved these changes Apr 29, 2021

View reviewed changes

update

5ca83ab

MaxGekk closed this in 11ea255 Apr 30, 2021

		@@ -93,6 +93,23 @@ object IntervalUtils {

		private val yearMonthPattern = "^([+\|-])?(\\d+)-(\\d+)$".r

		private val yearMonthFuzzyPattern = "[^-\|+]?([+\|-]?[\\d]+-[\\d]+).".r

	"^(INTERVAL\\s+)([+\|-])?(')([+\|-])?(\\d+)-(\\d+)(')(\\s+YEAR\\s+TO\\s+MONTH)$".r
	"(?i)^(INTERVAL\\s+)([+\|-])?(')([+\|-])?(\\d+)-(\\d+)(')(\\s+YEAR\\s+TO\\s+MONTH)$".r

	(c, evPrim, evNull) => code"$evPrim = $util.castStringToYMInterval($c);"
	(c, evPrim, _) => code"$evPrim = $util.castStringToYMInterval($c);"

[SPARK-35111][SQL] Support Cast string to year-month interval #32266

[SPARK-35111][SQL] Support Cast string to year-month interval #32266

Conversation

AngersZhuuuu commented Apr 21, 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?

AngersZhuuuu commented Apr 21, 2021

SparkQA commented Apr 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxGekk Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AngersZhuuuu Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this 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 21, 2021

SparkQA commented Apr 21, 2021

SparkQA commented Apr 22, 2021

SparkQA commented Apr 22, 2021

SparkQA commented Apr 22, 2021

SparkQA commented Apr 22, 2021

SparkQA commented Apr 22, 2021

SparkQA commented Apr 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Apr 23, 2021

SparkQA commented Apr 23, 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

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

SparkQA commented Apr 29, 2021

SparkQA commented Apr 29, 2021

SparkQA commented Apr 29, 2021

SparkQA commented Apr 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxGekk left a comment

Choose a reason for hiding this comment

SparkQA commented Apr 30, 2021

SparkQA commented Apr 30, 2021

MaxGekk commented Apr 30, 2021

SparkQA commented Apr 30, 2021

AngersZhuuuu commented Apr 21, 2021 •

edited

Loading

MaxGekk Apr 29, 2021 •

edited

Loading

AngersZhuuuu Apr 23, 2021 •

edited

Loading