dongjoon-hyun commented on code in PR #59956:
URL: https://github.com/apache/spark/pull/59956#discussion_r4424171870
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala:
##########
@@ -788,11 +836,15 @@ case class DivideYMInterval(
LongMath.divide(months, num.asInstanceOf[Long],
RoundingMode.HALF_UP).toInt
case _: IntegralType => (months: Int, num) =>
IntMath.divide(months, num.asInstanceOf[Number].intValue(),
RoundingMode.HALF_UP)
- case _: DecimalType => (months: Int, num) =>
+ case _: DecimalType => (months: Int, num) =>
checkRoundingOverflow(origin.context) {
val decimalRes = ((new Decimal).set(months) /
num.asInstanceOf[Decimal]).toJavaBigDecimal
decimalRes.setScale(0, java.math.RoundingMode.HALF_UP).intValueExact()
+ }
case _: FractionalType => (months: Int, num) =>
- DoubleMath.roundToInt(months / num.asInstanceOf[Number].doubleValue(),
RoundingMode.HALF_UP)
+ val quotient = months / num.asInstanceOf[Number].doubleValue()
+ checkRoundingOverflow(origin.context,
java.lang.Double.isFinite(quotient)) {
Review Comment:
**1. [Medium] A finite divisor can still raise the bare
`ArithmeticException` when the quotient is infinite**
`Double.isFinite(quotient)` is false for an infinite quotient as well as for
NaN, so this overflow still escapes without an error condition:
```sql
SELECT INTERVAL '1' DAY / 1e-290D; --
INTERVAL_ARITHMETIC_OVERFLOW.WITH_SUGGESTION
SELECT INTERVAL '1' DAY / 1e-300D; -- java.lang.ArithmeticException: input
is infinite or NaN
```
The quotients are `8.64E300` and `Infinity`. `INTERVAL '2147483647' MONTH /
1e-300D` behaves the same way.
An infinite quotient can only come from a finite divisor: a zero divisor is
rejected before this point, and an infinite divisor gives a quotient of 0. A
NaN quotient can only come from a NaN divisor. So `!quotient.isNaN` is the
check that keeps the NaN behavior and still reports every overflow. The same
applies to L894, L930 and L973, and the `finite` parameter and its doc at
L766-L767 would need a new name.
##########
docs/sql-migration-guide.md:
##########
@@ -28,6 +28,7 @@ license: |
## Upgrading from Spark SQL 4.3 to 4.4
+- Since Spark 4.4, dividing an ANSI interval by a number raises
`INTERVAL_ARITHMETIC_OVERFLOW.WITH_SUGGESTION` (SQLSTATE 22015) whenever the
result does not fit the interval type, in every evaluation mode. Previously,
dividing the minimum year-month or day-time interval by `-1` raised
`ARITHMETIC_OVERFLOW` (SQLSTATE 22003) under whole-stage codegen, and dividing
an interval by a decimal or a floating-point number whose rounded quotient
overflows the type raised a `java.lang.ArithmeticException` without an error
condition.
Review Comment:
**2. [Low] The migration note does not match the behavior in two places**
- "whenever the result does not fit the interval type, in every evaluation
mode" does not hold yet for the infinite quotient of item 1, e.g. `INTERVAL '1'
DAY / 1e-300D`. Fixing item 1 makes this sentence, and the PR title, correct.
- "under whole-stage codegen" is narrower than the old behavior. With
`spark.sql.codegen.wholeStage=false` and the default
`spark.sql.codegen.factoryMode`, a projection still uses generated code and
raises `ARITHMETIC_OVERFLOW`. That is why the PR description sets both configs
for its interpreted run. "in generated code" would be accurate.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala:
##########
@@ -744,6 +744,54 @@ trait IntervalDivide {
}
}
+ /**
+ * The codegen counterpart of `checkDivideOverflow`: the check that an
integral divisor does
+ * not overflow the interval type, which only the minimum value divided by
-1 does.
+ */
+ def checkDivideOverflowCodegen(
Review Comment:
**4. [Low] The PR description's account of SPARK-50226 does not match the
history**
The description says that SPARK-50226 changed the interpreted path to the
interval error class and missed the two inline copies in the generated code.
The two paths already differed before it. At `bd94419c988^`, the interpreted
path raised `intervalArithmeticOverflowError` (`INTERVAL_ARITHMETIC_OVERFLOW`),
and the generated code raised `overflowInIntegralDivideError`. SPARK-50226 only
moved the interpreted path to the `WITH_SUGGESTION` sub-condition. The
description becomes the commit message, so it is worth correcting.
The first sentence, "Under ANSI mode", could go too. These expressions do
not read `spark.sql.ansi.enabled`, and `nonansi/interval.sql.out` shows the
same error.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala:
##########
@@ -744,6 +744,54 @@ trait IntervalDivide {
}
}
+ /**
+ * The codegen counterpart of `checkDivideOverflow`: the check that an
integral divisor does
+ * not overflow the interval type, which only the minimum value divided by
-1 does.
+ */
+ def checkDivideOverflowCodegen(
+ value: String,
+ minValueLiteral: String,
+ num: String,
+ errorContextReference: String): String = {
+ s"""
+ |if ($value == $minValueLiteral && $num == -1)
+ | throw
QueryExecutionErrors.withSuggestionIntervalArithmeticOverflowError(
+ | "try_divide", $errorContextReference);
+ |""".stripMargin
+ }
+
+ /**
+ * Rounds the quotient of the interval by a decimal or a fractional divisor
in `round`, and
+ * raises the interval overflow error when the rounded quotient does not fit
the interval
+ * type: the rounding raises an `ArithmeticException` then. A fractional
quotient that is not
+ * `finite`, e.g. with a NaN divisor, is left to the rounding, which raises
for it as before.
+ */
+ def checkRoundingOverflow[T](context: QueryContext, finite: Boolean =
true)(round: => T): T = {
+ try {
Review Comment:
**6. [Nit] This is the same try/catch as `IntervalMathUtils.withOverflow`**
`IntervalMathUtils.withOverflow` also turns an `ArithmeticException` into
`withSuggestionIntervalArithmeticOverflowError`. It is private and passes
`context = null` today, so it can not be called as is, but a `context`
parameter would let both share it. This would come naturally with item 5.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala:
##########
@@ -812,32 +864,34 @@ case class DivideYMInterval(
}
val javaType = CodeGenerator.javaType(dataType)
nullSafeCodeGen(ctx, ev, (m, n) => {
- val checkIntegralDivideOverflow =
- s"""
- |if ($m == ${Int.MinValue} && $n == -1)
- | throw
QueryExecutionErrors.overflowInIntegralDivideError($errorContext);
- |""".stripMargin
// Similarly to non-codegen code. The result of `divide(Int, Long,
...)` must fit
// to `Int`. Casting to `Int` is safe here.
s"""
|${divideByZeroCheckCodegen(right.dataType, n, errorContext)}
- |$checkIntegralDivideOverflow
+ |${checkDivideOverflowCodegen(m, Int.MinValue.toString, n,
errorContext)}
|${ev.value} = ($javaType)$math.divide($m, $n,
java.math.RoundingMode.HALF_UP);
""".stripMargin
})
case _: DecimalType =>
nullSafeCodeGen(ctx, ev, (m, n) =>
s"""
|${divideByZeroCheckCodegen(right.dataType, n, errorContext)}
- |${ev.value} = ((new
Decimal()).set($m).$$div($n)).toJavaBigDecimal()
- | .setScale(0, java.math.RoundingMode.HALF_UP).intValueExact();
+ |${checkRoundingOverflowCodegen(
+ s"""
+ |${ev.value} = ((new
Decimal()).set($m).$$div($n)).toJavaBigDecimal()
+ | .setScale(0,
java.math.RoundingMode.HALF_UP).intValueExact();
+ """.stripMargin, errorContext)}
Review Comment:
**8. [Nit] The nested multi-line string could be a local val**
A multi-line `s"""..."""` with its own `stripMargin` inside the
interpolation of the outer string is hard to read, here and at L958-L962. A
local val, as the removed `checkIntegralDivideOverflow` was, keeps the outer
template flat:
```scala
nullSafeCodeGen(ctx, ev, (m, n) => {
val round =
s"""
|${ev.value} = ((new Decimal()).set($m).$$div($n)).toJavaBigDecimal()
| .setScale(0, java.math.RoundingMode.HALF_UP).intValueExact();
""".stripMargin
s"""
|${divideByZeroCheckCodegen(right.dataType, n, errorContext)}
|${checkRoundingOverflowCodegen(round, errorContext)}
""".stripMargin
})
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala:
##########
@@ -744,6 +744,54 @@ trait IntervalDivide {
}
}
+ /**
+ * The codegen counterpart of `checkDivideOverflow`: the check that an
integral divisor does
+ * not overflow the interval type, which only the minimum value divided by
-1 does.
+ */
+ def checkDivideOverflowCodegen(
+ value: String,
+ minValueLiteral: String,
+ num: String,
+ errorContextReference: String): String = {
+ s"""
+ |if ($value == $minValueLiteral && $num == -1)
+ | throw
QueryExecutionErrors.withSuggestionIntervalArithmeticOverflowError(
+ | "try_divide", $errorContextReference);
+ |""".stripMargin
+ }
+
+ /**
+ * Rounds the quotient of the interval by a decimal or a fractional divisor
in `round`, and
+ * raises the interval overflow error when the rounded quotient does not fit
the interval
+ * type: the rounding raises an `ArithmeticException` then. A fractional
quotient that is not
+ * `finite`, e.g. with a NaN divisor, is left to the rounding, which raises
for it as before.
+ */
+ def checkRoundingOverflow[T](context: QueryContext, finite: Boolean =
true)(round: => T): T = {
+ try {
+ round
+ } catch {
+ case _: ArithmeticException if finite =>
+ throw
QueryExecutionErrors.withSuggestionIntervalArithmeticOverflowError(
+ "try_divide", context)
+ }
+ }
+
+ /** The codegen counterpart of `checkRoundingOverflow`, `finite` being a
Java expression. */
+ def checkRoundingOverflowCodegen(
+ code: String,
+ errorContextReference: String,
+ finite: String = "true"): String = {
Review Comment:
**7. [Nit] The default `finite = "true"` adds dead code to the decimal path**
The decimal branches use this default, so their generated code contains `if
(!(true)) throw e;`. A decimal quotient has no non-finite case, so the
parameter only serves the fractional branches. It goes away with item 5. If the
helpers stay, the guard could be emitted only for the fractional branches.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/IntervalExpressionsSuite.scala:
##########
@@ -476,6 +476,26 @@ class IntervalExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
}
}
+ 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. The error carries an
empty query context,
+ // which `checkErrorInExpression` cannot match, so the whole message is
checked instead,
+ // including the suggested function.
+ val expectedErrMsg = "[INTERVAL_ARITHMETIC_OVERFLOW.WITH_SUGGESTION]
Integer overflow " +
+ "while operating with intervals. Use `try_divide` to tolerate overflow
and return NULL " +
+ "instead."
+ Seq(-1.toByte, -1.toShort, -1, -1L, Decimal(-1), -1.0f, -1.0d).foreach {
num =>
Review Comment:
**3. [Low] No test covers an overflow with an infinite quotient**
The only explicit cases that reach the `finite = false` branch are the
existing `Float.NaN` ones, so item 1 passes the tests. These two would catch it:
```scala
DivideYMInterval(Literal(Period.ofMonths(Int.MaxValue)), Literal(1e-300d))
DivideDTInterval(Literal(Duration.ofDays(1)), Literal(1e-300d))
```
An overflow of a value other than the minimum is only in the golden files,
with a year-month interval and a decimal divisor.
`DivideDTInterval(Literal(Duration.ofDays(1)), Literal(1e-290d))` would add a
day-time interval and a fractional divisor. These go beyond the title of this
test, so they may fit better in a separate test.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala:
##########
@@ -744,6 +744,54 @@ trait IntervalDivide {
}
}
+ /**
+ * The codegen counterpart of `checkDivideOverflow`: the check that an
integral divisor does
+ * not overflow the interval type, which only the minimum value divided by
-1 does.
+ */
+ def checkDivideOverflowCodegen(
+ value: String,
+ minValueLiteral: String,
+ num: String,
+ errorContextReference: String): String = {
+ s"""
+ |if ($value == $minValueLiteral && $num == -1)
+ | throw
QueryExecutionErrors.withSuggestionIntervalArithmeticOverflowError(
+ | "try_divide", $errorContextReference);
+ |""".stripMargin
+ }
+
+ /**
+ * Rounds the quotient of the interval by a decimal or a fractional divisor
in `round`, and
+ * raises the interval overflow error when the rounded quotient does not fit
the interval
+ * type: the rounding raises an `ArithmeticException` then. A fractional
quotient that is not
+ * `finite`, e.g. with a NaN divisor, is left to the rounding, which raises
for it as before.
+ */
+ def checkRoundingOverflow[T](context: QueryContext, finite: Boolean =
true)(round: => T): T = {
Review Comment:
**5. [Low] Could both evaluation paths call one function instead?**
The description names the separate copies of the check as the root cause,
but each path still has its own try/catch, `finite` decision and error name,
for both interval types. So the same drift is still possible.
`Add`, `Subtract` and `UnaryMinus` avoid this for intervals: the interpreted
path and the generated code both call `IntervalMathUtils.addExact` and the
like. `MakeYMInterval` does the same with `IntervalUtils.makeYearMonthInterval`
(L609, L616). Overloads such as `IntervalMathUtils.divide(months, num,
context)`, one per interval type and divisor kind, would keep the checks and
the error in one place. `doGenCode` would become a single call with no
generated try/catch.
--
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]
