From de3d4cba062f02189f9c1beee427db313cddc885 Mon Sep 17 00:00:00 2001 From: David Mollitor Date: Fri, 18 Sep 2026 17:07:48 +0000 Subject: [PATCH 1/2] [SPARK-59643][SQL] Tighten CheckOverflow nullability to match MakeDecimal CheckOverflow declared `nullable = true` unconditionally, while its sibling MakeDecimal (immediately above it in the same file) already declares the accurate form `child.nullable || nullOnOverflow`. CheckOverflow returns null only when its input is null, or when an overflow occurs with `nullOnOverflow = true` (under ANSI, `nullOnOverflow` is false and an overflow throws). Align CheckOverflow with MakeDecimal: override def nullable: Boolean = child.nullable || nullOnOverflow and, in doGenCode, only re-derive `isNull` from the value when the expression is nullable (mirroring MakeDecimal) -- otherwise nullSafeCodeGen makes `ev.isNull` a literal and the assignment would be invalid. No behavioral change: overflow still throws under ANSI and returns null otherwise. Only the declared nullability tightens, so a CheckOverflow-wrapped decimal over non-null inputs is now reported non-nullable under ANSI. Co-authored-by: Isaac --- .../sql/catalyst/expressions/decimalExpressions.scala | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala index f24c907681502..6d2f6ee5ed251 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala @@ -117,7 +117,9 @@ case class CheckOverflow( dataType: DecimalType, nullOnOverflow: Boolean) extends UnaryExpression with SupportQueryContext { - override def nullable: Boolean = true + // When `nullOnOverflow` is false, an overflow throws instead of producing null, so the result is + // null only when the input is. When true, an overflow yields null regardless of the input. + override def nullable: Boolean = child.nullable || nullOnOverflow override def nullSafeEval(input: Any): Any = input.asInstanceOf[Decimal].toPrecision( @@ -130,11 +132,14 @@ case class CheckOverflow( override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = { val errorContextCode = getContextOrNullCode(ctx, !nullOnOverflow) nullSafeCodeGen(ctx, ev, eval => { + // Mirror MakeDecimal above: `ev.isNull` is a literal when this expression is non-nullable + // (under `!nullOnOverflow` a non-null input never overflows to null -- it throws), so only + // re-derive it from the value when the expression is actually nullable. + val setIsNull = if (nullable) s"\n${ev.isNull} = ${ev.value} == null;" else "" // scalastyle:off line.size.limit s""" |${ev.value} = $eval.toPrecision( - | ${dataType.precision}, ${dataType.scale}, Decimal.ROUND_HALF_UP(), $nullOnOverflow, $errorContextCode); - |${ev.isNull} = ${ev.value} == null; + | ${dataType.precision}, ${dataType.scale}, Decimal.ROUND_HALF_UP(), $nullOnOverflow, $errorContextCode);$setIsNull """.stripMargin // scalastyle:on line.size.limit }) From 1682a0802c343e511bebca9f5a8dd715b923fc04 Mon Sep 17 00:00:00 2001 From: David Mollitor Date: Tue, 22 Sep 2026 16:43:03 +0000 Subject: [PATCH 2/2] [SPARK-59643][SQL] Address review: nullability test, codegen shape, comment Add a DecimalExpressionSuite test covering the tightened CheckOverflow nullability contract (including the successful non-nullable codegen path), mirror MakeDecimal's codegen shape (isNull assignment on its own line), and trim the redundant doGenCode comment. Co-authored-by: Isaac --- .../sql/catalyst/expressions/decimalExpressions.scala | 9 ++++----- .../catalyst/expressions/DecimalExpressionSuite.scala | 10 ++++++++++ 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala index 6d2f6ee5ed251..b383cd25ea4da 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala @@ -132,14 +132,13 @@ case class CheckOverflow( override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = { val errorContextCode = getContextOrNullCode(ctx, !nullOnOverflow) nullSafeCodeGen(ctx, ev, eval => { - // Mirror MakeDecimal above: `ev.isNull` is a literal when this expression is non-nullable - // (under `!nullOnOverflow` a non-null input never overflows to null -- it throws), so only - // re-derive it from the value when the expression is actually nullable. - val setIsNull = if (nullable) s"\n${ev.isNull} = ${ev.value} == null;" else "" + // Only assign isNull when nullable; nullSafeCodeGen makes it a literal otherwise. + val setIsNull = if (nullable) s"${ev.isNull} = ${ev.value} == null;" else "" // scalastyle:off line.size.limit s""" |${ev.value} = $eval.toPrecision( - | ${dataType.precision}, ${dataType.scale}, Decimal.ROUND_HALF_UP(), $nullOnOverflow, $errorContextCode);$setIsNull + | ${dataType.precision}, ${dataType.scale}, Decimal.ROUND_HALF_UP(), $nullOnOverflow, $errorContextCode); + |$setIsNull """.stripMargin // scalastyle:on line.size.limit }) diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DecimalExpressionSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DecimalExpressionSuite.scala index 513a62dc7f09c..82420a6d73a1a 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DecimalExpressionSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DecimalExpressionSuite.scala @@ -78,6 +78,16 @@ class DecimalExpressionSuite extends SparkFunSuite with ExpressionEvalHelper { Literal.create(null, DecimalType(2, 1)), DecimalType(3, 2), false), null) } + test("SPARK-59643: CheckOverflow nullability") { + val d = Literal(Decimal("10.1")) // non-nullable + val n = Literal.create(null, DecimalType(3, 1)) // nullable + assert(CheckOverflow(d, DecimalType(4, 1), nullOnOverflow = true).nullable) + assert(!CheckOverflow(d, DecimalType(4, 1), nullOnOverflow = false).nullable) + assert(CheckOverflow(n, DecimalType(4, 1), nullOnOverflow = true).nullable) + assert(CheckOverflow(n, DecimalType(4, 1), nullOnOverflow = false).nullable) + checkEvaluation(CheckOverflow(d, DecimalType(4, 1), nullOnOverflow = false), Decimal("10.1")) + } + test("SPARK-39208: CheckOverflow & CheckOverflowInSum support query context in runtime errors") { val d = Decimal(101, 3, 1) val query = "select cast(d as decimal(4, 3)) from t"