From b0609242c501b9ca8085e41f82c583eb96ba312f Mon Sep 17 00:00:00 2001 From: David Mollitor Date: Fri, 18 Sep 2026 16:19:02 +0000 Subject: [PATCH 1/3] [SPARK-59639][SQL] Tighten nullability of Divide, Remainder, IntegralDivide, and Pmod under ANSI mode Divide, Remainder, IntegralDivide (which share the DivModLike trait) and Pmod declared `nullable = true` unconditionally. Under ANSI mode (the default), divide-by-zero and integral overflow throw instead of returning null, so these operators are null only when one of their inputs is null. The over-broad nullability forces codegen to materialize an isNull flag and makes every parent expression emit a null-guard branch on it; because nullability propagates, one spuriously-nullable `%` cascades dead `if (!isNull)` guards into the comparisons, CASE WHENs, and predicates built on top of it. Make `nullable` reflect the runtime behavior: override def nullable: Boolean = left.nullable || right.nullable || !failOnError Under ANSI (failOnError) the result is null iff a child is. LEGACY/TRY are unchanged (still nullable): divide-by-zero returns null there, and for decimals a precision overflow can also return null independent of the divisor. No behavioral change: eval and code generation semantics are untouched (ANSI divide/remainder/pmod-by-zero and integral overflow still throw). Only the declared nullability tightens, and only under ANSI. Co-authored-by: Isaac --- .../spark/sql/catalyst/expressions/arithmetic.scala | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala index d0d7507e3250f..f8de904cbd7fb 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala @@ -682,7 +682,11 @@ trait DivModLike extends BinaryArithmetic { // Whether we should check overflow or not in ANSI mode. protected def checkDivideOverflow: Boolean = false - override def nullable: Boolean = true + // The result is null only when an input is, unless a divide/remainder-by-zero (or, for decimals, + // a precision overflow) can produce a null without failing. Under ANSI (`failOnError`) those + // conditions throw instead of returning null, so the result is null iff a child is. Non-ANSI / + // TRY stay conservatively nullable (zero -> null; decimal overflow -> null). + override def nullable: Boolean = left.nullable || right.nullable || !failOnError private lazy val isZero: Any => Boolean = right.dataType match { case _: DecimalType => x => x.asInstanceOf[Decimal].isZero @@ -1152,7 +1156,9 @@ case class Pmod( override def inputType: AbstractDataType = NumericType - override def nullable: Boolean = true + // See DivModLike.nullable: pmod-by-zero (and decimal precision overflow) throw under ANSI rather + // than returning null, so the result is null iff a child is; non-ANSI stays nullable. + override def nullable: Boolean = left.nullable || right.nullable || !failOnError override def decimalMethod: String = "remainder" From 911386687afb3b403943e40d9205d80c1de187bf Mon Sep 17 00:00:00 2001 From: David Mollitor Date: Fri, 18 Sep 2026 17:32:15 +0000 Subject: [PATCH 2/3] [SPARK-59639][SQL] Drop the now-dead isNull store when div/mod/pmod is non-nullable Tightening `nullable` in the previous commit removed the parent null-guard branches, but `DivModLike.doGenCode` / `Pmod.doGenCode` still emitted `boolean isNull = false;` unconditionally -- leaving a dead store, plus a dead local that widened the StackMapTable frames covering its scope, whenever the result is now non-nullable. Mirror MakeDecimal/CheckOverflow: in the non-null-input branch, when the result is non-nullable, omit the `isNull` declaration and report `FalseLiteral`, and guard Pmod's decimal `ev.isNull = value == null` line on `nullable`. On codegen-heavy workflows this drops generated class bytecode a further ~3% on top of the previous change (the largest stage class ~13% smaller), with no behavioral change. Co-authored-by: Isaac --- .../sql/catalyst/expressions/arithmetic.scala | 45 ++++++++++++++----- 1 file changed, 34 insertions(+), 11 deletions(-) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala index f8de904cbd7fb..d6933945f147b 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala @@ -816,11 +816,23 @@ trait DivModLike extends BinaryArithmetic { |} else {$divisionBody |}""".stripMargin } - ev.copy(code = code""" - ${eval2.code} - boolean ${ev.isNull} = false; - $javaType ${ev.value} = ${CodeGenerator.defaultValue(dataType)}; - $guardedBody""") + if (nullable) { + ev.copy(code = code""" + ${eval2.code} + boolean ${ev.isNull} = false; + $javaType ${ev.value} = ${CodeGenerator.defaultValue(dataType)}; + $guardedBody""") + } else { + // With non-null inputs and `failOnError`, divide-by-zero and overflow throw rather than + // producing null, so the result is non-nullable. Omit the dead `isNull` flag and report + // FalseLiteral (mirrors MakeDecimal/CheckOverflow). + ev.copy( + code = code""" + ${eval2.code} + $javaType ${ev.value} = ${CodeGenerator.defaultValue(dataType)}; + $guardedBody""", + isNull = FalseLiteral) + } } else { val nullOnErrorCondition = if (failOnError || divisorIsNonZero) "" else s" || $isZero" val failOnErrorBranch = if (failOnError && !divisorIsNonZero) { @@ -1242,7 +1254,7 @@ case class Pmod( |} |${ev.value} = ${ev.value}.toPrecision( | $precision, $scale, Decimal.ROUND_HALF_UP(), ${!failOnError}, $errorContext); - |${ev.isNull} = ${ev.value} == null; + |${if (nullable) s"${ev.isNull} = ${ev.value} == null;" else ""} |""".stripMargin // The positive-modulo arithmetic is the same fixed algorithm for every primitive numeric @@ -1274,11 +1286,22 @@ case class Pmod( |} else {$remainderBody |}""".stripMargin } - ev.copy(code = code""" - ${eval2.code} - boolean ${ev.isNull} = false; - $javaType ${ev.value} = ${CodeGenerator.defaultValue(dataType)}; - $guardedBody""") + if (nullable) { + ev.copy(code = code""" + ${eval2.code} + boolean ${ev.isNull} = false; + $javaType ${ev.value} = ${CodeGenerator.defaultValue(dataType)}; + $guardedBody""") + } else { + // See DivModLike: non-null inputs under `failOnError` make the result non-nullable, so omit + // the dead `isNull` flag and report FalseLiteral. + ev.copy( + code = code""" + ${eval2.code} + $javaType ${ev.value} = ${CodeGenerator.defaultValue(dataType)}; + $guardedBody""", + isNull = FalseLiteral) + } } else { val nullOnErrorCondition = if (failOnError || divisorIsNonZero) "" else s" || $isZero" val failOnErrorBranch = if (failOnError && !divisorIsNonZero) { From 81020378dd79293add9e505290db4e41b3992217 Mon Sep 17 00:00:00 2001 From: David Mollitor Date: Sun, 20 Sep 2026 14:15:12 +0000 Subject: [PATCH 3/3] [SPARK-59639][SQL] Update ExplainSuiteAE expectations for the elided isnotnull Tightening Divide/Remainder/Pmod nullability makes `id % ` non-nullable under ANSI, so InferFiltersFromConstraints no longer inserts the now-redundant isnotnull((id % 10)) predicate that inner-join key inference used to add. Update the two affected ExplainSuiteAE golden strings accordingly: - "SPARK-35884: Explain formatted with subquery": drop the AND isnotnull((id % 10)) conjunct from the Filter condition. - "SPARK-55052: ... coalesced and coalesced-skewed": renumber the AQEShuffleRead nodes (6 -> 5, 13 -> 11) after the removed filter nodes. Test-only change; no production behavior change. Co-authored-by: Isaac --- .../src/test/scala/org/apache/spark/sql/ExplainSuite.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/ExplainSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/ExplainSuite.scala index 618f0ab675e19..02c7f67b8ac89 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/ExplainSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/ExplainSuite.scala @@ -655,7 +655,7 @@ class ExplainSuiteAE extends ExplainSuiteHelper with EnableAdaptiveExecutionSuit """ |(2) Filter [codegen id : 2] |Input [1]: [id#xL] - |Condition : ((id#xL > Subquery subquery#x, [id=#x]) AND isnotnull((id#xL % 10))) + |Condition : (id#xL > Subquery subquery#x, [id=#x]) |""".stripMargin, """ |(6) BroadcastQueryStage @@ -971,7 +971,7 @@ class ExplainSuiteAE extends ExplainSuiteHelper with EnableAdaptiveExecutionSuit checkKeywordsExistsInExplain( df = df, mode = ExplainMode.fromString("FORMATTED"), - keywords = "AQEShuffleRead (6), coalesced", "AQEShuffleRead (13), coalesced and skewed") + keywords = "AQEShuffleRead (5), coalesced", "AQEShuffleRead (11), coalesced and skewed") } } }