[SPARK-36924][SQL] CAST between ANSI intervals and IntegralType #34494

Peng-Lei · 2021-11-05T11:27:45Z

What changes were proposed in this pull request?

Add cast AnsiIntervalType to IntegralType

requirement:

YearMonthIntervalType just have one unit
DayTimeIntervalType just have one unit

cast rule:

The value corresponding to the unit of YearMonthIntervalType is the value of the IntegralType after conversion.
The value corresponding to the unit of DayTimeIntervalType is the value of the IntegralType after conversion.

Add cast IntegralType to AnsiIntervalType
requirement:

YearMonthIntervalType just have one unit
DayTimeIntervalType just have one unit

cast rule:

The value of the IntegralTypeis the value of YearMonthIntervalType that with the single unit after conversion.
The value of the IntegralTypeis the value of DayTimeIntervalType that with the single unit after conversion.

Why are the changes needed?

According to 2011 Standards

If TD is an interval and SD is exact numeric, then TD shall contain only a single .
If TD is exact numeric and SD is an interval, then SD shall contain only a single .

Does this PR introduce any user-facing change?

Yes, user can use cast function between YearMonthIntervalType to NumericType

How was this patch tested?

add ut testcase

Peng-Lei · 2021-11-05T11:36:10Z

@MaxGekk @cloud-fan Can you give me some comments. At now I just done the cast between YearMonthInterval and Numeric. Your guidance will help me during development of casting between DayTimeIntervalType and Numeric.

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

SparkQA · 2021-11-05T12:31:19Z

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

SparkQA · 2021-11-05T13:14:40Z

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

SparkQA · 2021-11-05T16:39:09Z

Test build #144931 has finished for PR 34494 at commit bf9de6c.

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

Peng-Lei · 2021-11-11T07:28:28Z

@MaxGekk @cloud-fan If you have time, could you please review the code again?

SparkQA · 2021-11-11T08:03:54Z

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

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

MaxGekk · 2021-11-11T08:15:21Z

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

+      b => IntervalUtils.longToDayTimeInterval(
+        x.exactNumeric.asInstanceOf[Numeric[Any]].toLong(b), it.endField)


Why don't you handle overflow here like for year-month intervals?

No overflow is possible that convert byte/short/int/long to long. It's not like year-month intervals, because there is a case that long to int.

MaxGekk · 2021-11-11T08:17:13Z

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

@@ -598,6 +606,15 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit
      IntervalUtils.castStringToYMInterval(s, it.startField, it.endField))
    case _: YearMonthIntervalType => buildCast[Int](_, s =>
      IntervalUtils.periodToMonths(IntervalUtils.monthsToPeriod(s), it.endField))
+    case x: IntegralType if it.startField == it.endField =>


How does the error message look like when t.startField != it.endField. It would be nice if we say to users that according to the SQL standard the start and end fields should be the same.

Oh, Maybe I made a mistake that we no need check it.startField == it.endField, because it has called the canCast and return false if it.startField != it.endField when go here. So I think no need add error message here. Do you think so?

At now, if it.startField != it.endField the exception is as follow:

So I think no need add error message here

+1

MaxGekk · 2021-11-11T08:20:45Z

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

+      buildCast[Int](_, i => yearMonthIntervalToInt(i, e))
+  }
+
+  private[this] def dayTimeIntervalToInt(v: Long, endFiled: Byte): Int = {


Could you embed it as this function is used only once.

make sense.

MaxGekk · 2021-11-11T08:25:06Z

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

+      buildCast[Int](_, i => yearMonthIntervalToShort(i, e))
+  }
+
+  private[this] def dayTimeIntervalToShort(v: Long, endFiled: Byte): Short = {


The same. Please, embed it.

SparkQA · 2021-11-11T08:48:49Z

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

SparkQA · 2021-11-11T09:58:49Z

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

SparkQA · 2021-11-11T10:55:16Z

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

SparkQA · 2021-11-11T11:55:59Z

Test build #145090 has finished for PR 34494 at commit faeffbc.

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

SparkQA · 2021-11-11T12:05:31Z

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

SparkQA · 2021-11-11T12:13:25Z

Test build #145095 has finished for PR 34494 at commit 004cc67.

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

SparkQA · 2021-11-11T13:07:47Z

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

SparkQA · 2021-11-11T14:13:25Z

Test build #145104 has finished for PR 34494 at commit b170f78.

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

cloud-fan · 2021-11-12T07:31:40Z

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


    case (_: DayTimeIntervalType, _: DayTimeIntervalType) => true
    case (_: YearMonthIntervalType, _: YearMonthIntervalType) => true
+    case (DayTimeIntervalType(s, e), _: IntegralType) if s == e => true
+    case (YearMonthIntervalType(s, e), _: IntegralType) if s == e => true


Since we can cast INTERVAL DAY TO SECOND to INTERVAL SECOND, I think it's a reasonable and convenient extension to cast INTERVAL DAY TO SECOND to integral type directly. That said, we can allow casting any interval type to integral type. For casting integral to interval, we should still only allow single field.

Since we can cast INTERVAL DAY TO SECOND to INTERVAL SECOND, I think it's a reasonable and convenient extension to cast INTERVAL DAY TO SECOND to integral type directly.

So clever. You're absolutely right. I didn't think of this use case.

cloud-fan · 2021-11-12T07:36:45Z

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

@@ -598,6 +606,15 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit
      IntervalUtils.castStringToYMInterval(s, it.startField, it.endField))
    case _: YearMonthIntervalType => buildCast[Int](_, s =>
      IntervalUtils.periodToMonths(IntervalUtils.monthsToPeriod(s), it.endField))
+    case x: IntegralType if it.startField == it.endField =>


So I think no need add error message here

+1

cloud-fan · 2021-11-12T07:37:29Z

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

+    case x: IntegralType if it.startField == it.endField =>
+      b =>
+        val intValue = try {
+          x.exactNumeric.asInstanceOf[Numeric[Any]].toInt(b)


how about we let intToYearMonthInterval take long? then only intToYearMonthInterval need to do overflow check, not here.

make sense. It's a little weird intToYearMonthInterval take a parameter of long type. So I want to change the method name: intToYearMonthInterval => integralToYearMonthInterval and longToDayTimeInterval => integralToDayTimeInterval. Do you agree?

Peng-Lei · 2021-11-12T10:24:16Z

Update. @cloud-fan @MaxGekk If you have time, I'd like to ask you to take a look at it again. Thank you very much.

SparkQA · 2021-11-12T11:07:53Z

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

SparkQA · 2021-11-12T12:03:44Z

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

SparkQA · 2021-11-12T15:24:45Z

Test build #145156 has finished for PR 34494 at commit b624c2f.

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

SparkQA · 2021-11-15T03:16:43Z

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

Peng-Lei · 2021-11-17T03:13:06Z

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

+      assert(it.startField == it.endField)
+      assert(x != LongType)
+      b => IntervalUtils.intToDayTimeInterval(
+        x.integral.asInstanceOf[Integral[Any]].toInt(b), it.endField)


Merge byte/short/int into one case branch to reduce the code and add assert(x != LongType) although I think it won't get here for LongType

seems better to unify the code more

case x: IntegralType => assert(it.startField == it.endField) if (x == LongType) ... else ...

Peng-Lei · 2021-11-17T03:15:12Z

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

+      assert(it.startField == it.endField)
+      assert(x != LongType)
+      b => IntervalUtils.intToYearMonthInterval(
+        x.integral.asInstanceOf[Integral[Any]].toInt(b), it.endField)


cloud-fan · 2021-11-17T04:45:00Z

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

+        code"""
+          $evPrim = $util.intToDayTimeInterval($c, (byte)${it.endField});
+        """
+    case LongType =>


ditto, we can unify the two cases and write if-else

cloud-fan · 2021-11-17T04:46:22Z

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

+      ctx: CodegenContext,
+      endFiled: Byte,
+      integralType: String,
+      catalogType: String): CastFunction = {


this parameter is not used

cloud-fan · 2021-11-17T04:46:50Z

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

+      ctx: CodegenContext,
+      endFiled: Byte,
+      integralType: String,
+      catalogType: String): CastFunction = {


this parameter is not used

cloud-fan · 2021-11-17T04:47:42Z

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

+      integralType: String,
+      catalogType: String): CastFunction = {
+    val util = IntervalUtils.getClass.getCanonicalName.stripSuffix("$")
+    integralType match {


I think the caller side should just pass Int, Long, etc. and here we can simply write $util.dayTimeInterval$integralType

cloud-fan · 2021-11-17T04:49:22Z

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

+    val vInt = yearMonthIntervalToInt(v, endFiled)
+    val vShort = vInt.toShort
+    if (vInt != vShort) {
+      throw QueryExecutionErrors.castingCauseOverflowError(v, ShortType.catalogString);


not a blocker, but it will be great to print v with interval format, not just an integer.

Seems more intuitive that print v with interval format. A small burden is the need to add a parameter startField to build the interval format. eg INTERVAL 'v' startField TO endField.

SparkQA · 2021-11-17T05:22:09Z

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

SparkQA · 2021-11-17T06:06:01Z

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

Peng-Lei · 2021-11-17T07:30:52Z

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

+    case x: DayTimeIntervalType =>
+      castDayTimeIntervalToIntegralTypeCode(x.endField, "Long")
+    case x: YearMonthIntervalType =>
+      castYearMonthIntervalToIntegralTypeCode(x.endField, "Int")


Here we use Int instead of Long, as we use yearMonthIntervaltoInt().toLong instead of define the function yearMonthIntervaltoLong. also I think no problem that assign int value to long result.

SparkQA · 2021-11-17T08:20:07Z

Test build #145310 has finished for PR 34494 at commit 20c8e9f.

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

SparkQA · 2021-11-17T08:21:09Z

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

SparkQA · 2021-11-17T08:32:06Z

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

SparkQA · 2021-11-17T09:07:31Z

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

Peng-Lei · 2021-11-17T09:11:48Z

not a blocker, but it will be great to print v with interval format, not just an integer.

Although it is not a blocker. but with interval format is friendly and clear. So I update the PR.

Peng-Lei · 2021-11-17T09:14:22Z

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

+    intToYearMonthInterval(vInt, endField)
+  }
+
+  def yearMonthIntervalToInt(v: Int, startField: Byte, endField: Byte): Int = {


Here, startField is not used. but in order to unify code in codegen $evPrim = $util.yearMonthIntervalTo$integralType($c, (byte)$startField, (byte)$endField);.

SparkQA · 2021-11-17T09:15:47Z

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

SparkQA · 2021-11-17T10:29:12Z

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

SparkQA · 2021-11-17T11:17:00Z

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

SparkQA · 2021-11-17T12:29:30Z

Test build #145322 has finished for PR 34494 at commit 2ac9d11.

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

SparkQA · 2021-11-17T12:35:55Z

Test build #145321 has finished for PR 34494 at commit 276673a.

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

SparkQA · 2021-11-17T14:37:34Z

Test build #145327 has finished for PR 34494 at commit c974930.

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

cloud-fan · 2021-11-17T14:50:10Z

The GA failure is unrelated. Merging to master, thanks!

Peng-Lei · 2021-11-18T01:12:57Z

@cloud-fan Thank you very much for your very patient review

github-actions bot added the SQL label Nov 5, 2021

cloud-fan reviewed Nov 5, 2021

View reviewed changes

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

Peng-Lei force-pushed the SPARK-36924 branch 2 times, most recently from faeffbc to 004cc67 Compare November 11, 2021 07:11

MaxGekk reviewed Nov 11, 2021

View reviewed changes

Peng-Lei changed the title ~~[SPARK-36924][SQL] CAST between ANSI intervals and numerics~~ [SPARK-36924][SQL] CAST between ANSI intervals and IntegralType Nov 11, 2021

Add cast between AnsiIntervalType and NumericType

b170f78

Peng-Lei force-pushed the SPARK-36924 branch from 004cc67 to b170f78 Compare November 11, 2021 09:19

cloud-fan reviewed Nov 12, 2021

View reviewed changes

address comments

514d275

Peng-Lei force-pushed the SPARK-36924 branch from b624c2f to 514d275 Compare November 15, 2021 01:43

Peng-Lei commented Nov 17, 2021

View reviewed changes

cloud-fan reviewed Nov 17, 2021

View reviewed changes

cloud-fan approved these changes Nov 17, 2021

View reviewed changes

Peng-Lei force-pushed the SPARK-36924 branch from 20c8e9f to 276673a Compare November 17, 2021 07:27

Peng-Lei commented Nov 17, 2021

View reviewed changes

address again

2ac9d11

Peng-Lei force-pushed the SPARK-36924 branch from 276673a to 2ac9d11 Compare November 17, 2021 07:34

optimize error message for overflow

c974930

Peng-Lei commented Nov 17, 2021

View reviewed changes

cloud-fan closed this in 9553ed7 Nov 17, 2021

		b => IntervalUtils.longToDayTimeInterval(
		x.exactNumeric.asInstanceOf[Numeric[Any]].toLong(b), it.endField)

[SPARK-36924][SQL] CAST between ANSI intervals and IntegralType #34494

[SPARK-36924][SQL] CAST between ANSI intervals and IntegralType #34494

Conversation

Peng-Lei commented Nov 5, 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?

Peng-Lei commented Nov 5, 2021

SparkQA commented Nov 5, 2021

SparkQA commented Nov 5, 2021

SparkQA commented Nov 5, 2021

Peng-Lei commented Nov 11, 2021

SparkQA commented Nov 11, 2021

Choose a reason for hiding this comment

Peng-Lei Nov 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 11, 2021

SparkQA commented Nov 11, 2021

SparkQA commented Nov 11, 2021

SparkQA commented Nov 11, 2021

SparkQA commented Nov 11, 2021

SparkQA commented Nov 11, 2021

SparkQA commented Nov 11, 2021

SparkQA commented Nov 11, 2021

Choose a reason for hiding this comment

Peng-Lei Nov 12, 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

Peng-Lei commented Nov 12, 2021

SparkQA commented Nov 12, 2021

SparkQA commented Nov 12, 2021

SparkQA commented Nov 12, 2021

SparkQA commented Nov 15, 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

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 Nov 17, 2021

SparkQA commented Nov 17, 2021

Choose a reason for hiding this comment

SparkQA commented Nov 17, 2021

SparkQA commented Nov 17, 2021

SparkQA commented Nov 17, 2021

SparkQA commented Nov 17, 2021

Peng-Lei commented Nov 17, 2021

Choose a reason for hiding this comment

SparkQA commented Nov 17, 2021

SparkQA commented Nov 17, 2021

SparkQA commented Nov 17, 2021

SparkQA commented Nov 17, 2021

SparkQA commented Nov 17, 2021

SparkQA commented Nov 17, 2021

cloud-fan commented Nov 17, 2021

Peng-Lei commented Nov 18, 2021

Peng-Lei commented Nov 5, 2021 •

edited

Loading

Peng-Lei Nov 11, 2021 •

edited

Loading

Peng-Lei Nov 12, 2021 •

edited

Loading