[SPARK-36632][SQL] DivideYMInterval and DivideDTInterval should throw the same exception when divide by zero. #33889

beliefer · 2021-09-01T08:55:21Z

What changes were proposed in this pull request?

When dividing by zero, DivideYMInterval and DivideDTInterval output

java.lang.ArithmeticException
/ by zero

But, in ansi mode, select 2 / 0 will output

org.apache.spark.SparkArithmeticException
divide by zero

The behavior looks not inconsistent.

Why are the changes needed?

Make consistent behavior.

Does this PR introduce any user-facing change?

'Yes'.

How was this patch tested?

New tests.

SparkQA · 2021-09-01T09:53:17Z

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

SparkQA · 2021-09-01T10:02:06Z

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

SparkQA · 2021-09-01T10:13:24Z

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

SparkQA · 2021-09-01T10:23:02Z

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

SparkQA · 2021-09-01T11:15:00Z

Test build #142916 has finished for PR 33889 at commit 218824c.

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

SparkQA · 2021-09-01T11:25:50Z

Test build #142915 has finished for PR 33889 at commit 631da9e.

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

SparkQA · 2021-09-01T12:57:20Z

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

SparkQA · 2021-09-01T13:06:28Z

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

SparkQA · 2021-09-01T15:07:28Z

Test build #142920 has finished for PR 33889 at commit 0325bfc.

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

beliefer · 2021-09-02T06:12:26Z

ping @MaxGekk @cloud-fan @gengliangwang

MaxGekk

I believe that ANSI intervals don't have to support non-ANSI mode. @beliefer Could you clarify why it should be supported? I would just fix current behavior. It should be the same independently from the SQL config.

HyukjinKwon · 2021-09-02T06:24:44Z

I don't feel strongly but just to clarify I think this PR more aims to make other functions to compute internal types in ANSI style. ANSI mode is technically not the compliance of ANSI but more like ANSI-like or ANSI-style IIRC.

WDYT @gengliangwang and @cloud-fan on this support?

MaxGekk · 2021-09-02T06:29:33Z

... to make other functions to compute internal types in ANSI style.

My question is why the functions/expressions that work on ANSI interval should support non-ANSI mode. They are not legacy, what's the reason?

beliefer · 2021-09-02T06:59:39Z

@MaxGekk @HyukjinKwon The function try_divide with non-ansi mode and Analyzer replace try_divide with Divide and Analyzer replace Divide with DivideYMInterval later. so it will not correct.
Please refer to

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

Line 116 in 9c5bcac

this(left, right, TryEval(Divide(left, right, failOnError = true)))

and

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

Line 428 in 9c5bcac

case (_: YearMonthIntervalType, _) => DivideYMInterval(l, r)

gengliangwang · 2021-09-02T08:46:50Z

@beliefer could you clarify the exact inconsistency behavior in the PR description?

gengliangwang · 2021-09-02T08:50:57Z

OK, I got it.
There is similar discussion in #33751 (comment)

beliefer · 2021-09-02T09:05:28Z

@MaxGekk
DivideYMInterval not consider the ansi mode, which leads some issue:

Divide supports both ansi and non-ansi mode. DivideYMInterval only supports ansi mode. If DivideYMInterval does not need to support non-ansi mode, then it is very contradictory with the second issue.
If we use Divide with non-ansi mode, when Analyzer replacing Divide with DivideYMInterval, current code ignored the non-ansi mode . The behavior looks so strange.

Or I think, if ansi interval encounters failOnError, it should throw an unsupported exception.

MaxGekk · 2021-09-02T09:38:48Z

... , current code ignored the non-ansi mode . The behavior looks so strange.

I cannot agree with that. ANSI intervals are new feature. We don't have any legacy user code which could require to support non-ANSI behavior. Everywhere in the new expressions, we have already implemented strong (ANSI) mode. Now, you propose to add many new branches for non-ansi implementations. Should be a good reason for overcomplicating the code besides of "consistency" from your point of view.

beliefer · 2021-09-02T09:55:36Z

@MaxGekk in ansi mode, select 2 / 0 will output

org.apache.spark.SparkArithmeticException
divide by zero

but select interval '2' year / 0 will output

java.lang.ArithmeticException
/ by zero

The behavior looks not inconsistent. I just think select interval '2' year / 0 should output the same exception.

MaxGekk · 2021-09-02T10:03:05Z

but select interval '2' year / 0 will output
java.lang.ArithmeticException

Then let's throw org.apache.spark.SparkArithmeticException independently from the SQL config (ANSI or non-ANSI).

cloud-fan · 2021-09-03T08:12:59Z

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

@@ -612,6 +612,13 @@ case class DivideYMInterval(
  override def inputTypes: Seq[AbstractDataType] = Seq(YearMonthIntervalType, NumericType)
  override def dataType: DataType = YearMonthIntervalType()

+  @transient
+  private lazy val checkFunc: (Any) => Unit = right.dataType match {


nit: ... val divideByZeroCheck: Any => Unit = ...

cloud-fan · 2021-09-03T08:15:56Z

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

@@ -641,6 +649,11 @@ case class DivideYMInterval(
      val javaType = CodeGenerator.javaType(dataType)
      val months = left.genCode(ctx)
      val num = right.genCode(ctx)
+      val checkDivideByZero =


we can add

private def divideByZeroCheckCodegen(value: String): String = right.dataType match { case _: DecimalType => "if ($value.isZero()) throw ..." case _ => "if ($value == 0) throw ..." }

to avoid duplicate code here.

and we can move these util functions to IntervalDivide

SparkQA · 2021-09-03T09:10:20Z

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

SparkQA · 2021-09-03T09:38:20Z

Kubernetes integration test unable to build dist.

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

SparkQA · 2021-09-03T10:10:56Z

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

SparkQA · 2021-09-03T11:00:00Z

Test build #142965 has finished for PR 33889 at commit 823048a.

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

SparkQA · 2021-09-03T13:17:33Z

Test build #142974 has finished for PR 33889 at commit b00d8d8.

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

SparkQA · 2021-09-03T13:19:56Z

Test build #142976 has finished for PR 33889 at commit 7df29e5.

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

cloud-fan · 2021-09-06T08:51:47Z

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

@@ -612,6 +617,13 @@ case class DivideYMInterval(
  override def inputTypes: Seq[AbstractDataType] = Seq(YearMonthIntervalType, NumericType)
  override def dataType: DataType = YearMonthIntervalType()

+  @transient
+  private lazy val divideByZeroCheck: Any => Unit = right.dataType match {


can we move this to IntervalDivide as well?

I agree to Wenchen, let's move it to the common trait.

MaxGekk

LGTM except of one comment about moving code to the common trait.

MaxGekk · 2021-09-06T19:46:11Z

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

@@ -598,6 +598,11 @@ trait IntervalDivide {
      }
    }
  }
+
+  def divideByZeroCheckCodegen(expr: Expression, value: String): String = expr.dataType match {


nit: Expression is too general as the first parameter, you could pass DataType. Up to you.

SparkQA · 2021-10-07T06:50:58Z

Test build #143941 has started for PR 33889 at commit 7df29e5.

SparkQA · 2021-10-07T09:19:17Z

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

SparkQA · 2021-10-07T10:33:40Z

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

cloud-fan · 2021-10-08T09:20:11Z

@beliefer any updates?

beliefer · 2021-10-14T06:32:30Z

@beliefer any updates?

I'm so sorry for update late.

SparkQA · 2021-10-14T07:27:28Z

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

SparkQA · 2021-10-14T08:28:57Z

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

cloud-fan · 2021-10-14T09:05:12Z

thanks, merging to master/3.2!

… the same exception when divide by zero ### What changes were proposed in this pull request? When dividing by zero, `DivideYMInterval` and `DivideDTInterval` output ``` java.lang.ArithmeticException / by zero ``` But, in ansi mode, `select 2 / 0` will output ``` org.apache.spark.SparkArithmeticException divide by zero ``` The behavior looks not inconsistent. ### Why are the changes needed? Make consistent behavior. ### Does this PR introduce _any_ user-facing change? 'Yes'. ### How was this patch tested? New tests. Closes #33889 from beliefer/SPARK-36632. Lead-authored-by: gengjiaan <gengjiaan@360.cn> Co-authored-by: beliefer <beliefer@163.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit de0161a) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

beliefer · 2021-10-14T09:59:03Z

@cloud-fan @MaxGekk @gengliangwang @HyukjinKwon Thank you for review.

SparkQA · 2021-10-14T11:24:14Z

Test build #144241 has finished for PR 33889 at commit d202787.

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

… the same exception when divide by zero ### What changes were proposed in this pull request? When dividing by zero, `DivideYMInterval` and `DivideDTInterval` output ``` java.lang.ArithmeticException / by zero ``` But, in ansi mode, `select 2 / 0` will output ``` org.apache.spark.SparkArithmeticException divide by zero ``` The behavior looks not inconsistent. ### Why are the changes needed? Make consistent behavior. ### Does this PR introduce _any_ user-facing change? 'Yes'. ### How was this patch tested? New tests. Closes apache#33889 from beliefer/SPARK-36632. Lead-authored-by: gengjiaan <gengjiaan@360.cn> Co-authored-by: beliefer <beliefer@163.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit de0161a) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

DivideYMInterval should consider ansi mode.

631da9e

github-actions bot added the SQL label Sep 1, 2021

Update code

218824c

Update code

0325bfc

beliefer added 2 commits September 2, 2021 10:42

Update code

cc4d5be

Update code

97b646a

MaxGekk reviewed Sep 2, 2021

View reviewed changes

beliefer added 4 commits September 2, 2021 19:17

Update code

4600e4f

Update code

1eebc3a

Update code

0bd83da

Update code

89af674

cloud-fan reviewed Sep 3, 2021

View reviewed changes

beliefer added 2 commits September 3, 2021 16:32

Update code

a3e3059

Update code

7df29e5

beliefer mentioned this pull request Sep 3, 2021

[SPARK-36633][SQL] DivideDTInterval should consider ansi mode. #33891

Closed

cloud-fan reviewed Sep 6, 2021

View reviewed changes

MaxGekk approved these changes Sep 6, 2021

View reviewed changes

Update code

d202787

cloud-fan approved these changes Oct 14, 2021

View reviewed changes

cloud-fan closed this in de0161a Oct 14, 2021

[SPARK-36632][SQL] DivideYMInterval and DivideDTInterval should throw the same exception when divide by zero. #33889

[SPARK-36632][SQL] DivideYMInterval and DivideDTInterval should throw the same exception when divide by zero. #33889

Conversation

beliefer commented Sep 1, 2021 • edited

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 Sep 1, 2021

SparkQA commented Sep 1, 2021

SparkQA commented Sep 1, 2021

SparkQA commented Sep 1, 2021

SparkQA commented Sep 1, 2021

SparkQA commented Sep 1, 2021

SparkQA commented Sep 1, 2021

SparkQA commented Sep 1, 2021

SparkQA commented Sep 1, 2021

beliefer commented Sep 2, 2021

MaxGekk left a comment

Choose a reason for hiding this comment

HyukjinKwon commented Sep 2, 2021

MaxGekk commented Sep 2, 2021

beliefer commented Sep 2, 2021

gengliangwang commented Sep 2, 2021

gengliangwang commented Sep 2, 2021

beliefer commented Sep 2, 2021 • edited

MaxGekk commented Sep 2, 2021 • edited

beliefer commented Sep 2, 2021 • edited

MaxGekk commented Sep 2, 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 Sep 3, 2021

SparkQA commented Sep 3, 2021

SparkQA commented Sep 3, 2021

SparkQA commented Sep 3, 2021

SparkQA commented Sep 3, 2021

SparkQA commented Sep 3, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Oct 7, 2021

SparkQA commented Oct 7, 2021

SparkQA commented Oct 7, 2021

cloud-fan commented Oct 8, 2021

beliefer commented Oct 14, 2021

SparkQA commented Oct 14, 2021

SparkQA commented Oct 14, 2021

cloud-fan commented Oct 14, 2021

beliefer commented Oct 14, 2021

SparkQA commented Oct 14, 2021

beliefer commented Sep 1, 2021 •

edited

beliefer commented Sep 2, 2021 •

edited

MaxGekk commented Sep 2, 2021 •

edited

beliefer commented Sep 2, 2021 •

edited