MaxGekk commented on code in PR #59956:
URL: https://github.com/apache/spark/pull/59956#discussion_r4426264762
##########
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:
Thanks for checking the history, you are right. The description now says
that the interpreted path has raised the interval error class since the check
was added, that SPARK-50226 only moved it to the `WITH_SUGGESTION`
sub-condition, and that the generated code had its own copy of the check;
"Under ANSI mode" is gone, since the expressions do not read the config and the
non-ANSI golden file 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 = {
Review Comment:
Done in bda6facf7bc. `IntervalMathUtils.divide` has an overload per interval
type (months as `Int`, micros as `Long`) and kind of divisor (`Long`,
`Decimal`, `Double`), each with the division by zero check, the overflow checks
and the error with the query context. Both expressions call them from
`evalFunc`, and `doGenCode`, now shared in the `IntervalDivide` trait, is a
single `defineCodeGen` call that widens the divisor to the overload's type. The
trait's helpers and the generated try/catch are gone, as in `Add` and
`Subtract` with `addExact`.
##########
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:
Reused in bda6facf7bc: `withOverflow` takes a `context` parameter now, with
`null` as the default for the existing callers, and the decimal and fractional
divisions go through it with `"try_divide"` and the query context.
##########
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:
Gone with item 5 in bda6facf7bc: the decimal overloads wrap the rounding in
`withOverflow` without a NaN check, and only the `Double` overloads test the
quotient for NaN.
##########
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:
Gone with item 5 in bda6facf7bc: `doGenCode` is a one-line `defineCodeGen`
call, with no nested strings.
--
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]
