[SPARK-30341][SQL] Overflow check for interval arithmetic operations #26995

yaooqinn · 2019-12-24T03:48:48Z

What changes were proposed in this pull request?

For the interval arithmetic functions, e.g. add/subtract/negative/multiply/divide, enable overflow check when ANSI is on.
For multiply/divide, throw an exception when an overflow happens in spite of ANSI is on/off.
add/subtract/negative stay the same for backward compatibility.
divide by 0 throws ArithmeticException whether ANSI or not as same as numerics.
These behaviors fit the numeric type operations fully when ANSI is on.
These behaviors fit the numeric type operations fully when ANSI is off, except 2 and 4.

Why are the changes needed?

bug fix
ANSI support

Does this PR introduce any user-facing change?

When ANSI is on, interval add/subtract/negative/multiply/divide will overflow if any field overflows

How was this patch tested?

add unit tests

yaooqinn · 2019-12-24T03:49:56Z

cc: @cloud-fan @maropu @HyukjinKwon, thanks very much for reviewing

SparkQA · 2019-12-24T04:18:26Z

Test build #115685 has finished for PR 26995 at commit 829cfe7.

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

maropu · 2019-12-24T05:59:06Z

Why did you target interval only in this pr? If we change the overflow behaivour for arighmetic operatrions, I think we need to carefully handle it cuz it has impacts on the existing behaivours.

yaooqinn · 2019-12-24T06:04:13Z

Why did you target interval only in this pr? If we change the overflow behaivour for arighmetic operatrions, I think we need to carefully handle it cuz it has impacts on the existing behaivours.

Because the numeric types are handled in arithmetics properly in both ANSI or non-ANSI mode based on my check. This PR tries to fix the interval to follow what the numeric ones do. Thus, the behavior impacts are limited to the interval operations as the pr description says.

SparkQA · 2019-12-24T06:29:55Z

Test build #115690 has finished for PR 26995 at commit 516080c.

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

maropu · 2019-12-24T06:31:27Z

We cannot check it outside BinaryArithmetic like ~~numeric~~ decimal CheckOverflow? It seems this pr includes the same try-catch patterns.

yaooqinn · 2019-12-24T06:50:33Z

CheckOverflow is for decimal only, not all numerics

SparkQA · 2019-12-24T06:57:05Z

Test build #115692 has finished for PR 26995 at commit 67767c0.

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

cloud-fan · 2019-12-24T07:05:16Z

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

@@ -37,6 +37,11 @@ case class UnaryMinus(child: Expression) extends UnaryExpression
    with ExpectsInputTypes with NullIntolerant {
  private val checkOverflow = SQLConf.get.ansiEnabled

+  override def nullable: Boolean = dataType match {
+    case CalendarIntervalType if !checkOverflow => true
+    case _ => super.nullable


nit: child.nullable is more clear here.

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

cloud-fan · 2019-12-24T07:09:55Z

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

@@ -218,6 +252,11 @@ object BinaryArithmetic {
  """)
 case class Add(left: Expression, right: Expression) extends BinaryArithmetic {

+  override def nullable: Boolean = dataType match {
+    case CalendarIntervalType if !checkOverflow => true


shall we make the overflow behavior consistent? e.g. other numeric types follow the java overflow behavior and interval returns null for overflow, which is inconsistent.

Yes, the current behavior of master is separating a) decimal(which is null for overflow) from b) other types. This pr (so far)is just adding intervals to group a). We may reach an agreement on which way to follow first.

in spark 2.4, do we have internal arithmetic operations? The non-ANSI behavior should follow the old behavior.

Yes, we have.
we now have +/-/unaray_- in java overflow behavior(2.4 or maybe earlier)
and we have '*/-' in null for overflow behavior (3.0)

then let's be consistent and follow java overflow behavior when ansi is false.

spark-sql> select cast('128' as tinyint); NULL Time taken: 0.029 seconds, Fetched 1 row(s) spark-sql> select cast(128 as tinyint); -128

not quite related to this pr, the cast logic seems unconsisent too

cast from string and cast from int are different and don't need to be consistent. But interval +- and interval */ should be consistent.

changed behavior to a) ANSI on: exception, b) ANSI off: java style overflow

SparkQA · 2019-12-24T08:05:02Z

Test build #115703 has finished for PR 26995 at commit 7293377.

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

SparkQA · 2019-12-24T12:00:10Z

Test build #115735 has finished for PR 26995 at commit f100d88.

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

SparkQA · 2019-12-24T13:03:41Z

Test build #115737 has finished for PR 26995 at commit ba44c5a.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class DivideInterval(

SparkQA · 2019-12-24T15:06:18Z

Test build #115740 has finished for PR 26995 at commit 67645d4.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class MultiplyInterval(interval: Expression, num: Expression)
case class DivideInterval(interval: Expression, num: Expression)

SparkQA · 2019-12-24T21:10:31Z

Test build #115746 has finished for PR 26995 at commit 7671d83.

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 · 2019-12-27T08:05:02Z

Test build #115847 has finished for PR 26995 at commit b679381.

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

yaooqinn · 2019-12-27T08:14:10Z

retest this please

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

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

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

Test build #116013 has finished for PR 26995 at commit aba10b2.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class MultiplyInterval(interval: Expression, num: Expression)
case class DivideInterval(interval: Expression, num: Expression)

cloud-fan · 2020-01-02T08:44:33Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Average.scala

@@ -81,7 +80,8 @@ case class Average(child: Expression) extends DeclarativeAggregate with Implicit
    case _: DecimalType =>
      DecimalPrecision.decimalAndDecimal(sum / count.cast(DecimalType.LongDecimal)).cast(resultType)
    case CalendarIntervalType =>
-      DivideInterval(sum.cast(resultType), count.cast(DoubleType))
+      val newCount = If(EqualTo(count, Literal(0L)), Literal(null, LongType), count)


avg(interval) is also new in 3.0 right? We can also fail here if this is the SQL standard.

I checked pgsql, avg on empty table returns null. So this is corrected.

cloud-fan · 2020-01-02T08:54:54Z

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

+              MultiplyInterval(Literal(stringToInterval(interval)), Literal(num)), expected)
+          } else {
+            checkEvaluation(MultiplyInterval(Literal(stringToInterval(interval)), Literal(num)),
+              if (expected == null) null else stringToInterval(expected))


can expected be null?

cloud-fan · 2020-01-02T08:55:14Z

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

+        withSQLConf(SQLConf.ANSI_ENABLED.key -> v) {
+          if (checkException) {
+            checkExceptionInExpression[ArithmeticException](
+              MultiplyInterval(Literal(stringToInterval(interval)), Literal(num)), expected)


we can add a val expr = MultiplyInterval(Literal(stringToInterval(interval)), Literal(num)) at beginning, to save code duplication.

yes, we can also use safeStringToInterval(expected) == null to choose way to check too

cloud-fan · 2020-01-02T08:55:28Z

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

+        withSQLConf(SQLConf.ANSI_ENABLED.key -> v) {
+          if (checkException) {
+            checkExceptionInExpression[ArithmeticException](
+              DivideInterval(Literal(stringToInterval(interval)), Literal(num)), expected)


SparkQA · 2020-01-02T09:02:28Z

Test build #116021 has finished for PR 26995 at commit 988b51c.

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

SparkQA · 2020-01-02T12:41:28Z

Test build #116023 has finished for PR 26995 at commit f80f0f3.

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

yaooqinn · 2020-01-02T12:44:21Z

retest this please

cloud-fan · 2020-01-02T12:53:46Z

seems multiple PRs start to fail org.apache.spark.sql.kafka010.KafkaDelegationTokenSuite. cc @HeartSaVioR

HeartSaVioR · 2020-01-02T13:21:16Z

@cloud-fan Thanks for pinging me. I'll take a look at these failures.

SparkQA · 2020-01-02T17:05:09Z

Test build #116038 has finished for PR 26995 at commit f80f0f3.

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

cloud-fan · 2020-01-02T18:04:33Z

thanks, merging to master!

HeartSaVioR · 2020-01-08T12:53:25Z

Just FYI I spent my time a bit to deal with org.apache.spark.sql.kafka010.KafkaDelegationTokenSuite, but no luck. I'd like to ask @gaborgsomogyi to help resolving the issue, as he authored the test and had investigated before for similar issue.

Hi @gaborgsomogyi , welcome back! Would you mind to try dealing with this flaky test? Thanks in advance!

gaborgsomogyi · 2020-01-08T13:14:06Z

@HeartSaVioR Thanks for pinging. I've had a slight look and seems like it may caused by kafka-clients 2.4 upgrade. Instead of ZkUtils now KafkaZkClient used to call getAllBrokersInCluster which is causing the issue:

Caused by: sbt.ForkMain$ForkError: org.apache.zookeeper.KeeperException$AuthFailedException: KeeperErrorCode = AuthFailed for /brokers/ids
	at org.apache.zookeeper.KeeperException.create(KeeperException.java:130)
	at org.apache.zookeeper.KeeperException.create(KeeperException.java:54)
	at kafka.zookeeper.AsyncResponse.resultException(ZooKeeperClient.scala:554)
	at kafka.zk.KafkaZkClient.getChildren(KafkaZkClient.scala:719)
	at kafka.zk.KafkaZkClient.getSortedBrokerList(KafkaZkClient.scala:455)
	at kafka.zk.KafkaZkClient.getAllBrokersInCluster(KafkaZkClient.scala:404)
	at org.apache.spark.sql.kafka010.KafkaTestUtils.$anonfun$setup$3(KafkaTestUtils.scala:293)
	at org.scalatest.concurrent.Eventually.makeAValiantAttempt$1(Eventually.scala:395)
	at org.scalatest.concurrent.Eventually.tryTryAgain$1(Eventually.scala:409)
	... 20 more

My first guess is that the new API behaves somehow different and/or it may contain some bugs.

gaborgsomogyi · 2020-01-08T13:15:30Z

Going to speak with the Kafka guys what they think about this...

yaooqinn · 2020-01-08T13:18:25Z

Found ticket for zkclient/localhost@EXAMPLE.COM to go to krbtgt/EXAMPLE.COM@EXAMPLE.COM expiring on Fri Jan 03 04:30:09 PST 2020
Service ticket not found in the subject
KrbException: Server not found in Kerberos database (7) - Server not found in Kerberos database
	at sun.security.krb5.KrbTgsRep.<init>(KrbTgsRep.java:73)
	at sun.security.krb5.KrbTgsReq.getReply(KrbTgsReq.java:251)
	at sun.security.krb5.KrbTgsReq.sendAndGetCreds(KrbTgsReq.java:262)
	at sun.security.krb5.internal.CredentialsUtil.serviceCreds(CredentialsUtil.java:308)
	at sun.security.krb5.internal.CredentialsUtil.acquireServiceCreds(CredentialsUtil.java:126)
	at sun.security.krb5.Credentials.acquireServiceCreds(Credentials.java:458)
	at sun.security.jgss.krb5.Krb5Context.initSecContext(Krb5Context.java:693)
	at sun.security.jgss.GSSContextImpl.initSecContext(GSSContextImpl.java:248)
	at sun.security.jgss.GSSContextImpl.initSecContext(GSSContextImpl.java:179)
	at com.sun.security.sasl.gsskerb.GssKrb5Client.evaluateChallenge(GssKrb5Client.java:192)
	at org.apache.zookeeper.client.ZooKeeperSaslClient$1.run(ZooKeeperSaslClient.java:323)
	at org.apache.zookeeper.client.ZooKeeperSaslClient$1.run(ZooKeeperSaslClient.java:320)
	at java.security.AccessController.doPrivileged(Native Method)
	at javax.security.auth.Subject.doAs(Subject.java:422)
	at org.apache.zookeeper.client.ZooKeeperSaslClient.createSaslToken(ZooKeeperSaslClient.java:320)
	at org.apache.zookeeper.client.ZooKeeperSaslClient.createSaslToken(ZooKeeperSaslClient.java:305)
	at org.apache.zookeeper.client.ZooKeeperSaslClient.sendSaslPacket(ZooKeeperSaslClient.java:377)
	at org.apache.zookeeper.client.ZooKeeperSaslClient.initialize(ZooKeeperSaslClient.java:415)
	at org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1149)
Caused by: KrbException: Identifier doesn't match expected value (906)
	at sun.security.krb5.internal.KDCRep.init(KDCRep.java:140)
	at sun.security.krb5.internal.TGSRep.init(TGSRep.java:65)
	at sun.security.krb5.internal.TGSRep.<init>(TGSRep.java:60)
	at sun.security.krb5.KrbTgsRep.<init>(KrbTgsRep.java:55)
	... 18 more

Hi @gaborgsomogyi you might need look at this exception which is throwed right before the AuthFailedException

gaborgsomogyi · 2020-01-08T13:47:46Z

@yaooqinn thanks for sharing. Yep, maybe the kerberos principal is wrong again. Not sure how this can be flaky though since it's hardcoded...

The mentioned AuthFailed is just the end-result of the KrbException.

yaooqinn · 2020-01-08T13:58:59Z

https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/116249/console here is another flaky failure, maybe related to Jenkins env, e.g. DNS, I'm not sure

gatorsmile · 2020-01-24T01:04:31Z

When ANSI is on, interval add/subtract/negative/multiply/divide will overflow if any field overflows. However, when ANSI is off and overflow happens, why multiply/divide will throw an exception but add/subtract/negative will not throw an exception? This looks inconsistent.

gatorsmile · 2020-01-24T01:10:02Z

sql/core/src/test/resources/sql-tests/results/interval.sql.out

+struct<>
+-- !query 117 output
+java.lang.ArithmeticException
+integer overflow


Can we change it and make them consistent like what you discussed above #26995 (comment)?

I am OK with your suggestion，but first let us also cc @cloud-fan

Please create a 3.0 blocker JIRA.

IIRC a problem is: the new operators in 3.0 (interval * double and interval / double) do not have a reasonable java style overflow behavior.

The actual operation is (int * double).toInt, and Double.PositiveInfinity.toInt returns Int.Max, which can be confusing.

Since ansi mode is off by default, we are introducing a new behavior that is non-ansi, which is weird. There are 2 options:

interval * double and interval / double return max or min int/long value when overflow. interval / 0 returns null.

revert these 2 operators.

@gatorsmile what do you think?

It sounds like the option 2 looks good to me.

gatorsmile · 2020-01-24T01:20:08Z

Compared with the previous release, any behavior change we made in this PR?

yaooqinn · 2020-01-24T01:23:36Z

Compared with the previous release, any behavior change we made in this PR?

When ANSI is on, interval add/subtract/negative will check overflow compared with 2.4

gatorsmile · 2020-01-24T04:57:51Z

Since the mode ANSI is introduced in Spark 3.0 and off by default, this is not a behavior change

[SPARK-30341][SQL] Overflow check for interval arithmetic operations

829cfe7

yaooqinn added 2 commits December 24, 2019 13:34

override nullable, fix tests

516080c

fix

67767c0

cloud-fan reviewed Dec 24, 2019

View reviewed changes

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

cloud-fan reviewed Dec 24, 2019

View reviewed changes

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

cloud-fan reviewed Dec 24, 2019

View reviewed changes

fix tests

7293377

use old overflow style

f100d88

yaooqinn added 2 commits December 24, 2019 20:17

workaround for avg

ba44c5a

divide 0 return null as other types

67645d4

fix tests

7671d83

cloud-fan reviewed Dec 27, 2019

View reviewed changes

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

name with exact

b679381

cloud-fan reviewed Dec 27, 2019

View reviewed changes

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

cloud-fan reviewed Jan 2, 2020

View reviewed changes

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

yaooqinn added 2 commits January 2, 2020 16:35

unify

e37caaf

blank lines

988b51c

cloud-fan reviewed Jan 2, 2020

View reviewed changes

refine tests

f80f0f3

cloud-fan closed this in e04309c Jan 2, 2020

gatorsmile reviewed Jan 24, 2020

View reviewed changes

dongjoon-hyun added the SQL label Feb 5, 2020

[SPARK-30341][SQL] Overflow check for interval arithmetic operations #26995

[SPARK-30341][SQL] Overflow check for interval arithmetic operations #26995

Conversation

yaooqinn commented Dec 24, 2019 • 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?

yaooqinn commented Dec 24, 2019

SparkQA commented Dec 24, 2019

maropu commented Dec 24, 2019

yaooqinn commented Dec 24, 2019 • edited Loading

SparkQA commented Dec 24, 2019

maropu commented Dec 24, 2019 • edited Loading

yaooqinn commented Dec 24, 2019

SparkQA commented Dec 24, 2019

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 Dec 24, 2019

SparkQA commented Dec 24, 2019

SparkQA commented Dec 24, 2019

SparkQA commented Dec 24, 2019

SparkQA commented Dec 24, 2019

SparkQA commented Dec 27, 2019

yaooqinn commented Dec 27, 2019

SparkQA commented Jan 2, 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

SparkQA commented Jan 2, 2020

SparkQA commented Jan 2, 2020

yaooqinn commented Jan 2, 2020

cloud-fan commented Jan 2, 2020 • edited Loading

HeartSaVioR commented Jan 2, 2020

SparkQA commented Jan 2, 2020

cloud-fan commented Jan 2, 2020

HeartSaVioR commented Jan 8, 2020

gaborgsomogyi commented Jan 8, 2020

gaborgsomogyi commented Jan 8, 2020

yaooqinn commented Jan 8, 2020

gaborgsomogyi commented Jan 8, 2020 • edited Loading

yaooqinn commented Jan 8, 2020

gatorsmile commented Jan 24, 2020

Choose a reason for hiding this comment

yaooqinn Jan 24, 2020 • 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

gatorsmile commented Jan 24, 2020

yaooqinn commented Jan 24, 2020

gatorsmile commented Jan 24, 2020

yaooqinn commented Dec 24, 2019 •

edited

Loading

yaooqinn commented Dec 24, 2019 •

edited

Loading

maropu commented Dec 24, 2019 •

edited

Loading

cloud-fan commented Jan 2, 2020 •

edited

Loading

gaborgsomogyi commented Jan 8, 2020 •

edited

Loading

yaooqinn Jan 24, 2020 •

edited

Loading