Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
yaooqinn wants to merge 20 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
829cfe7
[SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn Dec 24, 2019
516080c
override nullable, fix tests
yaooqinn Dec 24, 2019
67767c0
fix
yaooqinn Dec 24, 2019
7293377
fix tests
yaooqinn Dec 24, 2019
f100d88
use old overflow style
yaooqinn Dec 24, 2019
ba44c5a
workaround for avg
yaooqinn Dec 24, 2019
67645d4
divide 0 return null as other types
yaooqinn Dec 24, 2019
7671d83
fix tests
yaooqinn Dec 24, 2019
b679381
name with exact
yaooqinn Dec 27, 2019
a8d29b6
rm try-catch
yaooqinn Dec 27, 2019
c3a8531
Merge branch 'master' into SPARK-30341
yaooqinn Dec 30, 2019
ccb1fd5
regen g f
yaooqinn Dec 30, 2019
d704c74
..Exact for ansi off
yaooqinn Dec 30, 2019
512570e
regen g f
yaooqinn Dec 30, 2019
5651959
import
yaooqinn Dec 30, 2019
92e2668
clean up
yaooqinn Dec 31, 2019
aba10b2
address comments
yaooqinn Jan 2, 2020
e37caaf
unify
yaooqinn Jan 2, 2020
988b51c
blank lines
yaooqinn Jan 2, 2020
f80f0f3
refine tests
yaooqinn Jan 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
unify
  • Loading branch information
@yaooqinn
yaooqinn committed Jan 2, 2020
commit e37caaf4115da2cdb7843b4595ec779bad9342b2
6 changes: 3 additions & 3 deletions ...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Average.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,9 @@

package org.apache.spark.sql.catalyst.expressions.aggregate

import org.apache.spark.sql.catalyst.analysis.{DecimalPrecision, FunctionRegistry, TypeCheckResult}
import org.apache.spark.sql.catalyst.analysis.{DecimalPrecision, FunctionRegistry}
import org.apache.spark.sql.catalyst.dsl.expressions._
import org.apache.spark.sql.catalyst.expressions._
import org.apache.spark.sql.catalyst.util.TypeUtils
import org.apache.spark.sql.types._

@ExpressionDescription(
Expand Down Expand Up @@ -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)
Copy link
Contributor

@cloud-fan cloud-fan Jan 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@cloud-fan cloud-fan Jan 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

DivideInterval(sum.cast(resultType), newCount.cast(DoubleType))
case _ =>
sum.cast(resultType) / count.cast(resultType)
}
Expand Down
9 changes: 2 additions & 7 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,8 @@ case class UnaryMinus(child: Expression) extends UnaryExpression
"""})
case _: CalendarIntervalType =>
val iu = IntervalUtils.getClass.getCanonicalName.stripSuffix("$")
defineCodeGen(ctx, ev,
interval => if (checkOverflow) {
s"$iu.negateExact($interval)"
} else {
s"$iu.negate($interval)"
}
)
val method = if (checkOverflow) "negateExact" else "negate"
defineCodeGen(ctx, ev, c => s"$iu.$method($c)")
}

protected override def nullSafeEval(input: Any): Any = dataType match {
Expand Down
36 changes: 12 additions & 24 deletions ...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,11 @@ object ExtractIntervalPart {
}
}

abstract class IntervalNumOperation(interval: Expression, num: Expression, operationName: String)
abstract class IntervalNumOperation(
interval: Expression,
num: Expression,
operation: (CalendarInterval, Double) => CalendarInterval,
operationName: String)
extends BinaryExpression with ImplicitCastInputTypes with Serializable {

override def left: Expression = interval
Expand All @@ -122,39 +126,23 @@ abstract class IntervalNumOperation(interval: Expression, num: Expression, opera

override def nullable: Boolean = true

override def nullSafeEval(interval: Any, num: Any): Any = operationName match {
case "multiply" =>
multiplyExact(interval.asInstanceOf[CalendarInterval], num.asInstanceOf[Double])
case "divide" =>
if (num == 0) return null
divideExact(interval.asInstanceOf[CalendarInterval], num.asInstanceOf[Double])
override def nullSafeEval(interval: Any, num: Any): Any = {
operation(interval.asInstanceOf[CalendarInterval], num.asInstanceOf[Double])
}

override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val methodStr: String =
IntervalUtils.getClass.getName.stripSuffix("$") + "." + operationName + "Exact"
operationName match {
case "multiply" => defineCodeGen(ctx, ev, (interval, num) => s"$methodStr($interval, $num)")
case "divide" => nullSafeCodeGen(ctx, ev, (interval, num) => {
s"""
|if ($num == 0) {
| ${ev.isNull} = true;
|} else {
| ${ev.value} = $methodStr($interval, $num);
|}
|""".stripMargin
})
}
val iu = IntervalUtils.getClass.getName.stripSuffix("$")
defineCodeGen(ctx, ev, (interval, num) => s"$iu.$operationName($interval, $num)")
}

override def prettyName: String = operationName + "_interval"
override def prettyName: String = operationName.stripSuffix("Exact") + "_interval"
}

case class MultiplyInterval(interval: Expression, num: Expression)
extends IntervalNumOperation(interval, num, "multiply")
extends IntervalNumOperation(interval, num, multiplyExact, "multiplyExact")

case class DivideInterval(interval: Expression, num: Expression)
extends IntervalNumOperation(interval, num, "divide")
extends IntervalNumOperation(interval, num, divideExact, "divideExact")

// scalastyle:off line.size.limit
@ExpressionDescription(
Expand Down
12 changes: 5 additions & 7 deletions ...t/src/test/scala/org/apache/spark/sql/catalyst/expressions/IntervalExpressionsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,7 @@ class IntervalExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
withSQLConf(SQLConf.ANSI_ENABLED.key -> v) {
if (checkException) {
checkExceptionInExpression[ArithmeticException](
MultiplyInterval(Literal(stringToInterval(interval)), Literal(num)),
"integer overflow")
MultiplyInterval(Literal(stringToInterval(interval)), Literal(num)), expected)
Copy link
Contributor

@cloud-fan cloud-fan Jan 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@yaooqinn yaooqinn Jan 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@cloud-fan cloud-fan Jan 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can expected be null?

Expand All @@ -225,7 +224,7 @@ class IntervalExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
check("-100 years -1 millisecond", 0.5, "-50 years -500 microseconds")
check("2 months 4 seconds", -0.5, "-1 months -2 seconds")
check("1 month 2 microseconds", 1.5, "1 months 15 days 3 microseconds")
check("2 months", Int.MaxValue, null, checkException = true)
check("2 months", Int.MaxValue, "integer overflow", checkException = true)
}

test("divide") {
Expand All @@ -238,8 +237,7 @@ class IntervalExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
withSQLConf(SQLConf.ANSI_ENABLED.key -> v) {
if (checkException) {
checkExceptionInExpression[ArithmeticException](
DivideInterval(Literal(stringToInterval(interval)), Literal(num)),
"integer overflow")
DivideInterval(Literal(stringToInterval(interval)), Literal(num)), expected)
Copy link
Contributor

@cloud-fan cloud-fan Jan 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

} else {
checkEvaluation(DivideInterval(Literal(stringToInterval(interval)), Literal(num)),
if (expected == null) null else stringToInterval(expected))
Expand All @@ -255,8 +253,8 @@ class IntervalExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
check("2 years -8 seconds", 0.5, "4 years -16 seconds")
check("-1 month 2 microseconds", -0.25, "4 months -8 microseconds")
check("1 month 3 microsecond", 1.5, "20 days 2 microseconds")
check("1 second", 0, null)
check(s"${Int.MaxValue} months", 0.9, null, checkException = true)
check("1 second", 0, "divide by zero", checkException = true)
check(s"${Int.MaxValue} months", 0.9, "integer overflow", checkException = true)
}

test("make interval") {
Expand Down
5 changes: 3 additions & 2 deletions sql/core/src/test/resources/sql-tests/results/ansi/interval.sql.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,9 +207,10 @@ struct<divide_interval(subtracttimestamps(TIMESTAMP '2019-10-15 00:00:00', TIMES
-- !query 25
select interval '2 seconds' / 0
-- !query 25 schema
struct<divide_interval(INTERVAL '2 seconds', CAST(0 AS DOUBLE)):interval>
struct<>
-- !query 25 output
NULL
java.lang.ArithmeticException
divide by zero


-- !query 26
Expand Down
5 changes: 3 additions & 2 deletions sql/core/src/test/resources/sql-tests/results/interval.sql.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,9 +207,10 @@ struct<divide_interval(subtracttimestamps(TIMESTAMP '2019-10-15 00:00:00', TIMES
-- !query 25
select interval '2 seconds' / 0
-- !query 25 schema
struct<divide_interval(INTERVAL '2 seconds', CAST(0 AS DOUBLE)):interval>
struct<>
-- !query 25 output
NULL
java.lang.ArithmeticException
divide by zero


-- !query 26
Expand Down