[SPARK-30919][SQL] Make interval multiply and divide's overflow behavior consistent with other operations #27672

yaooqinn · 2020-02-21T18:23:31Z

What changes were proposed in this pull request?

The current behavior of interval multiply and divide follows the ANSI SQL standard when overflow, it is compatible with other operations when spark.sql.ansi.enabled is true, but not compatible when spark.sql.ansi.enabled is false.

When spark.sql.ansi.enabled is false, as the factor is a double value, so it should use java's rounding or truncation behavior for casting double to integrals. when divided by zero, it returns null. we also follow the natural rules for intervals as defined in the Gregorian calendar, so we do not add the month fraction to days but add days fraction to microseconds.

Why are the changes needed?

Make interval multiply and divide's overflow behavior consistent with other interval operations

Does this PR introduce any user-facing change?

no, these are new features in 3.0

How was this patch tested?

add uts

…ior consisitent with other interval operations

SparkQA · 2020-02-21T22:45:48Z

Test build #118803 has finished for PR 27672 at commit ae9869c.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
abstract class IntervalNumOperation(interval: Expression, num: Expression)

SparkQA · 2020-02-22T13:45:44Z

Test build #118814 has finished for PR 27672 at commit a05af57.

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

yaooqinn · 2020-02-24T03:35:39Z

cc @cloud-fan @gatorsmile, thanks for reviewing.

cloud-fan · 2020-02-24T09:09:47Z

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

+  extends IntervalNumOperation(interval, num) {
+
+  override protected def operation(interval: CalendarInterval, num: Double): CalendarInterval = {
+    if (checkOverflow) multiplyExact(interval, num) else multiply(interval, num)


we shouldn't check the ansi flag for each row.

we should define operation as a val: val operation: (CalendarInterval, Double) => CalendarInterval

cloud-fan · 2020-02-24T09:28:37Z

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

+    } else if (daysWithFraction < Int.MinValue) {
+      Int.MinValue
+    } else {
+      daysWithFraction.toInt


what if we just call daysWithFraction.toInt?

cloud-fan · 2020-02-24T10:27:18Z

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

+  extends IntervalNumOperation(interval, num) {
+
+  override protected val operation: (CalendarInterval, Double) => CalendarInterval =
+    (interval, num) => if (checkOverflow) multiplyExact(interval, num) else multiply(interval, num)


This still checks overflow per-row...

if (checkOverflow) multiplyExact else multiply

cloud-fan · 2020-02-24T10:28:12Z

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

+  extends IntervalNumOperation(interval, num) {
+
+  override protected val operation: (CalendarInterval, Double) => CalendarInterval =
+    (interval, num) => if (checkOverflow) divideExact(interval, num) else divide(interval, num)


cloud-fan · 2020-02-24T10:32:57Z

LGTM except one comment

cloud-fan · 2020-02-24T13:23:58Z

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

  extends BinaryExpression with ImplicitCastInputTypes with Serializable {
  override def left: Expression = interval
  override def right: Expression = num

+  protected val checkOverflow: Boolean = SQLConf.get.ansiEnabled


can we follow https://github.com/apache/spark/pull/27683/files#diff-1516b10738479bbe190fb4e239258473R36 and make it a parameter?

SparkQA · 2020-02-24T14:25:17Z

Test build #118860 has finished for PR 27672 at commit 0c82efe.

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

cloud-fan · 2020-02-24T15:24:44Z

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

@@ -308,12 +308,12 @@ class Analyzer(
          case _ => s
        }
        case m @ Multiply(l, r) if m.childrenResolved => (l.dataType, r.dataType) match {
-          case (CalendarIntervalType, _) => MultiplyInterval(l, r)
-          case (_, CalendarIntervalType) => MultiplyInterval(r, l)
+          case (CalendarIntervalType, _) => MultiplyInterval(l, r, conf.ansiEnabled)


SQLConf.get is reliable now. Let's follow other PRs and use the default parameter value.

SparkQA · 2020-02-24T16:13:20Z

Test build #118865 has finished for PR 27672 at commit 2bf07a3.

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

SparkQA · 2020-02-24T18:39:37Z

Test build #118871 has finished for PR 27672 at commit ebb0011.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class MultiplyInterval(
case class DivideInterval(

SparkQA · 2020-02-24T21:14:12Z

Test build #118878 has finished for PR 27672 at commit e96135f.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
abstract class IntervalNumOperation(

cloud-fan · 2020-02-25T04:20:23Z

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

@@ -113,12 +114,14 @@ object ExtractIntervalPart {
 abstract class IntervalNumOperation(
    interval: Expression,
    num: Expression,
-    operation: (CalendarInterval, Double) => CalendarInterval,
-    operationName: String)
+    val checkOverflow: Boolean = SQLConf.get.ansiEnabled)


hey wait. Does it mean this value can change when we transform/copy MultiplyInterval/DivideInterval?

cloud-fan · 2020-02-25T07:04:48Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/IntervalUtilsSuite.scala

+      assert(new CalendarInterval(0, 0, -15 * MICROS_PER_SECOND) === func(interval, 2))
+      assert(new CalendarInterval(-2, 0, -MICROS_PER_MINUTE) === func(interval, 0.5))
+    }
+    assert(divide(new CalendarInterval(123, 456, 789), 0) === null)


can we test one more case that divide returns Int.Max when overflow?

cloud-fan · 2020-02-25T07:05:58Z

...lyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/IntervalExpressionsSuite.scala

+        interval: String,
+        num: Double,
+        expected: String,
+        configs: Seq[String] = Seq("true", "false")): Unit = {


nit: isAnsi: Option[Boolean]? If it's None then we test both configs.

SparkQA · 2020-02-25T08:05:01Z

Test build #118898 has finished for PR 27672 at commit 7ece786.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-02-25T08:05:02Z

Test build #118906 has finished for PR 27672 at commit 1a67228.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-02-25T08:07:13Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/IntervalUtilsSuite.scala

+      assert(new CalendarInterval(-2, 0, -MICROS_PER_MINUTE) === func(interval, 0.5))
+    }
+
+    assert(divide(new CalendarInterval(Int.MaxValue, Int.MaxValue, 0), 0.9) ===


we should test divideExact as well.

cloud-fan · 2020-02-25T08:08:25Z

...lyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/IntervalExpressionsSuite.scala

      val expectedRes = safeStringToInterval(expected)
-      Seq("true", "false").foreach { v =>
+      val configs = if (isAnsi.isEmpty) { Seq("true", "false") } else isAnsi.map(_.toString).toSeq


super nit: { Seq("true", "false") } -> Seq("true", "false")

cloud-fan · 2020-02-25T08:22:31Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/IntervalUtilsSuite.scala

+    assert(divide(interval, 0.9) === new CalendarInterval(Int.MaxValue, Int.MaxValue,
+      ((Int.MaxValue / 9.0) * MICROS_PER_DAY).round))
+    try {
+      divideExact(interval, 0.9)


we should use intercept to assert a failure. I'm OK to leave it as we already use try-catch in this test case.

SparkQA · 2020-02-25T13:49:15Z

Test build #118910 has finished for PR 27672 at commit 9db1f37.

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

cloud-fan · 2020-02-25T14:19:39Z

thanks, merging to master/3.0!

cloud-fan · 2020-02-25T14:20:09Z

feel free to address #27672 (comment) in your next PR :)

…ior consistent with other operations ### What changes were proposed in this pull request? The current behavior of interval multiply and divide follows the ANSI SQL standard when overflow, it is compatible with other operations when `spark.sql.ansi.enabled` is true, but not compatible when `spark.sql.ansi.enabled` is false. When `spark.sql.ansi.enabled` is false, as the factor is a double value, so it should use java's rounding or truncation behavior for casting double to integrals. when divided by zero, it returns `null`. we also follow the natural rules for intervals as defined in the Gregorian calendar, so we do not add the month fraction to days but add days fraction to microseconds. ### Why are the changes needed? Make interval multiply and divide's overflow behavior consistent with other interval operations ### Does this PR introduce any user-facing change? no, these are new features in 3.0 ### How was this patch tested? add uts Closes #27672 from yaooqinn/SPARK-30919. Authored-by: Kent Yao <yaooqinn@hotmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 761209c) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…t failures in IntervalUtilsSuite ### What changes were proposed in this pull request? In this PR, I addressed the comment from #27672 (comment) to use `intercept` instead of `try-catch` block to assert failures in the IntervalUtilsSuite ### Why are the changes needed? improve tests ### Does this PR introduce any user-facing change? no ### How was this patch tested? Nah Closes #27700 from yaooqinn/intervaltest. Authored-by: Kent Yao <yaooqinn@hotmail.com> Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org>

…t failures in IntervalUtilsSuite ### What changes were proposed in this pull request? In this PR, I addressed the comment from #27672 (comment) to use `intercept` instead of `try-catch` block to assert failures in the IntervalUtilsSuite ### Why are the changes needed? improve tests ### Does this PR introduce any user-facing change? no ### How was this patch tested? Nah Closes #27700 from yaooqinn/intervaltest. Authored-by: Kent Yao <yaooqinn@hotmail.com> Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org> (cherry picked from commit 2d2706c) Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org>

…ior consistent with other operations ### What changes were proposed in this pull request? The current behavior of interval multiply and divide follows the ANSI SQL standard when overflow, it is compatible with other operations when `spark.sql.ansi.enabled` is true, but not compatible when `spark.sql.ansi.enabled` is false. When `spark.sql.ansi.enabled` is false, as the factor is a double value, so it should use java's rounding or truncation behavior for casting double to integrals. when divided by zero, it returns `null`. we also follow the natural rules for intervals as defined in the Gregorian calendar, so we do not add the month fraction to days but add days fraction to microseconds. ### Why are the changes needed? Make interval multiply and divide's overflow behavior consistent with other interval operations ### Does this PR introduce any user-facing change? no, these are new features in 3.0 ### How was this patch tested? add uts Closes apache#27672 from yaooqinn/SPARK-30919. Authored-by: Kent Yao <yaooqinn@hotmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…t failures in IntervalUtilsSuite ### What changes were proposed in this pull request? In this PR, I addressed the comment from apache#27672 (comment) to use `intercept` instead of `try-catch` block to assert failures in the IntervalUtilsSuite ### Why are the changes needed? improve tests ### Does this PR introduce any user-facing change? no ### How was this patch tested? Nah Closes apache#27700 from yaooqinn/intervaltest. Authored-by: Kent Yao <yaooqinn@hotmail.com> Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org>

[SPARK-30919][SQL] Make interval multiply and divide's overflow behav…

ae9869c

…ior consisitent with other interval operations

yaooqinn requested a review from cloud-fan February 21, 2020 18:23

nit

a05af57

cloud-fan reviewed Feb 24, 2020

View reviewed changes

address comments

0c82efe

cloud-fan reviewed Feb 24, 2020

View reviewed changes

address comments

2bf07a3

cloud-fan reviewed Feb 24, 2020

View reviewed changes

checkOverflow as para

ebb0011

cloud-fan reviewed Feb 24, 2020

View reviewed changes

address comments

e96135f

cloud-fan approved these changes Feb 24, 2020

View reviewed changes

cloud-fan reviewed Feb 25, 2020

View reviewed changes

fix potential value change during case class copy

7ece786

cloud-fan reviewed Feb 25, 2020

View reviewed changes

add more tests

1a67228

cloud-fan reviewed Feb 25, 2020

View reviewed changes

cloud-fan approved these changes Feb 25, 2020

View reviewed changes

add more tests

9db1f37

cloud-fan reviewed Feb 25, 2020

View reviewed changes

cloud-fan closed this in 761209c Feb 25, 2020

yaooqinn mentioned this pull request Feb 26, 2020

[SPARK-30956][SQL][TESTS] Use intercept instead of try-catch to assert failures in IntervalUtilsSuite #27700

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-30919][SQL] Make interval multiply and divide's overflow behavior consistent with other operations #27672

[SPARK-30919][SQL] Make interval multiply and divide's overflow behavior consistent with other operations #27672

yaooqinn commented Feb 21, 2020

SparkQA commented Feb 21, 2020

SparkQA commented Feb 22, 2020

yaooqinn commented Feb 24, 2020

cloud-fan Feb 24, 2020

cloud-fan Feb 24, 2020

yaooqinn Feb 24, 2020

cloud-fan Feb 24, 2020

yaooqinn Feb 24, 2020

cloud-fan Feb 24, 2020

cloud-fan Feb 24, 2020

cloud-fan commented Feb 24, 2020

cloud-fan Feb 24, 2020

SparkQA commented Feb 24, 2020

cloud-fan Feb 24, 2020

SparkQA commented Feb 24, 2020

SparkQA commented Feb 24, 2020

SparkQA commented Feb 24, 2020

cloud-fan Feb 25, 2020

cloud-fan Feb 25, 2020

cloud-fan Feb 25, 2020

SparkQA commented Feb 25, 2020

SparkQA commented Feb 25, 2020

cloud-fan Feb 25, 2020

cloud-fan Feb 25, 2020

cloud-fan Feb 25, 2020

SparkQA commented Feb 25, 2020

cloud-fan commented Feb 25, 2020

cloud-fan commented Feb 25, 2020

[SPARK-30919][SQL] Make interval multiply and divide's overflow behavior consistent with other operations #27672

[SPARK-30919][SQL] Make interval multiply and divide's overflow behavior consistent with other operations #27672

Conversation

yaooqinn commented Feb 21, 2020

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?

SparkQA commented Feb 21, 2020

SparkQA commented Feb 22, 2020

yaooqinn commented Feb 24, 2020

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

cloud-fan commented Feb 24, 2020

Choose a reason for hiding this comment

SparkQA commented Feb 24, 2020

Choose a reason for hiding this comment

SparkQA commented Feb 24, 2020

SparkQA commented Feb 24, 2020

SparkQA commented Feb 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Feb 25, 2020

SparkQA commented Feb 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Feb 25, 2020

cloud-fan commented Feb 25, 2020

cloud-fan commented Feb 25, 2020