dongjoon-hyun commented on code in PR #59956:
URL: https://github.com/apache/spark/pull/59956#discussion_r4470601423
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/IntervalExpressionsSuite.scala:
##########
@@ -476,6 +476,45 @@ class IntervalExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
}
}
+ // The error of an interval division that overflows. It carries an empty
query context, which
+ // `checkErrorInExpression` cannot match, so the whole message is checked,
with the suggested
+ // function.
+ private val intervalDivideOverflowErrMsg =
+ "[INTERVAL_ARITHMETIC_OVERFLOW.WITH_SUGGESTION] Integer overflow while
operating with " +
+ "intervals. Use `try_divide` to tolerate overflow and return NULL
instead."
+
+ test("SPARK-60746: an interval division whose quotient does not fit raises
the overflow error") {
+ // A quotient that is out of the range of the interval type once rounded,
or infinite, from
+ // a decimal or a fractional divisor, in every mode. A NaN quotient is not
an overflow, see
+ // the SPARK-34868 and SPARK-34875 tests.
+ Seq(
+ DivideYMInterval(Literal(Period.ofMonths(Int.MaxValue)),
Literal(Decimal(0.5))),
+ DivideYMInterval(Literal(Period.ofMonths(Int.MaxValue)),
Literal(1e-300d)),
+ DivideDTInterval(Literal(Duration.ofDays(1)), Literal(1e-290d)),
+ DivideDTInterval(Literal(Duration.ofDays(1)), Literal(1e-300d))
+ ).foreach { expr =>
+ withClue(s"$expr: ") {
+ checkExceptionInExpression[SparkArithmeticException](expr,
intervalDivideOverflowErrMsg)
+ }
+ }
+ }
+
+ test("SPARK-60746: the min interval divided by -1 raises the same error in
every mode") {
+ // An integral divisor is checked before the division, a decimal or a
fractional one when the
+ // rounded quotient does not fit the interval type.
+ val expectedErrMsg = intervalDivideOverflowErrMsg
Review Comment:
**11. [Nit] `expectedErrMsg` only renames `intervalDivideOverflowErrMsg`**
Could the two calls below use `intervalDivideOverflowErrMsg` directly, as
the test above does?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala:
##########
@@ -729,42 +729,22 @@ case class MultiplyDTInterval(
copy(interval = newLeft, num = newRight)
}
-trait IntervalDivide {
- def checkDivideOverflow(
- value: Any,
- minValue: Any,
- num: Expression,
- numValue: Any,
- context: QueryContext): Unit = {
- if (value == minValue && num.dataType.isInstanceOf[IntegralType]) {
- if (numValue.asInstanceOf[Number].longValue() == -1) {
- throw
QueryExecutionErrors.withSuggestionIntervalArithmeticOverflowError(
- "try_divide", context)
- }
+/**
+ * The division of an interval by a number. The interpreted path and the
generated code both call
+ * the `IntervalMathUtils.divide` overload for the interval type and the kind
of the divisor, which
+ * holds the checks and the error, so that the two paths cannot drift apart.
+ */
+trait IntervalDivide extends BinaryExpression {
Review Comment:
**10. [Nit] `IntervalDivide` already extends `BinaryExpression`**
Now that this trait extends `BinaryExpression`, the `extends` lines of
`DivideYMInterval` (L755) and `DivideDTInterval` (L789) repeat it, and their
declarations no longer show that the shared `doGenCode` comes from this trait.
Could they be `extends IntervalDivide with ImplicitCastInputTypes with
Serializable`?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalMathUtils.scala:
##########
@@ -35,15 +41,86 @@ object IntervalMathUtils {
def negateExact(a: Long): Long = withOverflow(Math.negateExact(a))
- private def withOverflow[A](f: => A, suggestedFunc: String = ""): A = {
+ // The divisions of a year-month interval, in months, and of a day-time
interval, in micros, by
+ // a number, rounded half up. The interpreted and the generated code of
`DivideYMInterval` and
+ // `DivideDTInterval` both call them, so that the checks and the error are
in one place. A zero
+ // divisor raises `INTERVAL_DIVIDED_BY_ZERO`, and a quotient that does not
fit the interval
+ // type raises the interval overflow error with `try_divide` suggested: the
minimum interval
+ // divided by -1, or a decimal or fractional quotient out of the range once
rounded, an infinite
+ // one included. A NaN quotient, from a NaN divisor, is not an overflow and
is left to the
+ // rounding, which raises for it.
+
+ def divide(months: Int, num: Long, context: QueryContext): Int = {
+ checkDivisor(num == 0, context)
+ if (months == Int.MinValue && num == -1) {
+ throw QueryExecutionErrors.withSuggestionIntervalArithmeticOverflowError(
+ "try_divide", context)
+ }
+ // The quotient of an Int by a Long fits an Int.
+ LongMath.divide(months, num, RoundingMode.HALF_UP).toInt
+ }
+
+ def divide(months: Int, num: Decimal, context: QueryContext): Int = {
+ checkDivisor(num.isZero, context)
+ withOverflow(
+ ((new Decimal).set(months) / num).toJavaBigDecimal
+ .setScale(0, RoundingMode.HALF_UP).intValueExact(),
+ "try_divide", context)
+ }
+
+ def divide(months: Int, num: Double, context: QueryContext): Int = {
+ checkDivisor(num == 0, context)
+ val quotient = months / num
+ if (quotient.isNaN) {
+ DoubleMath.roundToInt(quotient, RoundingMode.HALF_UP)
+ } else {
+ withOverflow(DoubleMath.roundToInt(quotient, RoundingMode.HALF_UP),
"try_divide", context)
+ }
+ }
+
+ def divide(micros: Long, num: Long, context: QueryContext): Long = {
+ checkDivisor(num == 0, context)
+ if (micros == Long.MinValue && num == -1) {
+ throw QueryExecutionErrors.withSuggestionIntervalArithmeticOverflowError(
+ "try_divide", context)
+ }
+ LongMath.divide(micros, num, RoundingMode.HALF_UP)
+ }
+
+ def divide(micros: Long, num: Decimal, context: QueryContext): Long = {
+ checkDivisor(num.isZero, context)
+ withOverflow(
+ ((new Decimal).set(micros) / num).toJavaBigDecimal
+ .setScale(0, RoundingMode.HALF_UP).longValueExact(),
+ "try_divide", context)
+ }
+
+ def divide(micros: Long, num: Double, context: QueryContext): Long = {
+ checkDivisor(num == 0, context)
+ val quotient = micros / num
+ if (quotient.isNaN) {
+ DoubleMath.roundToLong(quotient, RoundingMode.HALF_UP)
+ } else {
+ withOverflow(DoubleMath.roundToLong(quotient, RoundingMode.HALF_UP),
"try_divide", context)
+ }
+ }
+
+ private def checkDivisor(isZero: Boolean, context: QueryContext): Unit = {
+ if (isZero) throw QueryExecutionErrors.intervalDividedByZeroError(context)
+ }
+
+ private def withOverflow[A](
+ f: => A,
+ suggestedFunc: String = "",
+ context: QueryContext = null): A = {
Review Comment:
**9. [Low] The `null` default of the new `context` parameter hides a missing
context**
Following up on item 6: with `context: QueryContext = null` as the default,
a caller that leaves out the context still compiles, and its error has no query
context. `addExact`, `subtractExact` and `negateExact` (L32-L42) are such
callers today. So on the same `VALUES` table in interval.sql.out, the error of
`a / 0.5` has a `queryContext` with the fragment `a / 0.5`, while the error of
`b + interval '1 month'` has none. Could `context` have no default, with
L32-L42 passing `null` explicitly for now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
