[SPARK-59639][SQL] Tighten nullability of Divide, Remainder, IntegralDivide, and Pmod under ANSI mode - #58910
Open
david-mollitor-db wants to merge 2 commits into
Open
david-mollitor-db wants to merge 2 commits into
david-mollitor-db wants to merge 2 commits into
Conversation
…Divide, 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 <no-reply@databricks.com>
…s 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 <no-reply@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Divide,Remainder, andIntegralDivide(which share theDivModLiketrait) andPmoddeclaredoverride def nullable: Boolean = trueunconditionally. This PR makesnullablereflect what the operators can actually produce at runtime:Under ANSI mode (
failOnError), divide-by-zero and integral overflow throw rather than returningnull, so the result isnullonly when one of the inputs isnull. Under LEGACY/TRY mode the behavior is unchanged (nullablestays effectivelytrue), because divide-by-zero returnsnullthere and, for decimals, a precision overflow can also returnnullindependent of the divisor.A follow-up commit also updates
DivModLike.doGenCode/Pmod.doGenCode: when the expression is now non-nullable they no longer emit theboolean isNull = false;declaration (which had become a dead store, plus a dead local that widened the StackMapTable frames covering its scope) and instead reportFalseLiteral— mirroring the existingMakeDecimal/CheckOverflowcodegen.evalsemantics are untouched.Why are the changes needed?
Whole-stage and expression code generation represent SQL
NULLwith booleanisNullflags. Anullable = truechild forces the framework to materialize anisNullvariable and makes every parent expression emit a null-guard branch on it (CodegenContext.nullSafeExec). Because nullability propagates, a single spuriously-nullable%///pmodcascades deadif (!isNull)branches into the comparisons,CASE WHENs, and predicates built on top of it.For example,
(id % 2) = 0inside aCASE WHEN(ANSI) generated:Correcting the nullability removes these dead guards at the source and feeds more accurate nullability to the optimizer; the codegen follow-up additionally removes the now-dead
isNull = falsestores.Beyond cleaner code, this has a JIT dimension. HotSpot does not JIT-compile a method whose bytecode exceeds
-XX:HugeMethodLimit(default 8000 bytes) — such a method runs in the interpreter. The per-operationisNullflags, null-guard branches, and dead stores inflate the bytecode of exactly the arithmetic-heavy generated methods that sit closest to this limit. On a set of representative codegen-heavy projections, the largest generated method dropped from ~9.1 KB (above the limit, interpreter-only) to ~7.7 KB (JIT-compiled) with this change.Does this PR introduce any user-facing change?
Yes (schema metadata only). Under ANSI mode, an output column that is a
/,%,div, orpmodover non-nullable inputs is now reported as non-nullable, whereas before it was reported as nullable. Computed values and error behavior are unchanged: ANSI divide/remainder/pmod-by-zero and integral overflow still throw, and LEGACY/TRY still returnnull.spark.range(1).selectExpr("id % 2").schema.head.nullableistruefalseHow was this patch tested?
Existing suites, all pass with no golden-file changes:
ArithmeticExpressionSuite,PredicateSuite,ColumnExpressionSuiteSQLQueryTestSuite:operators.sql,try_arithmetic.sql,decimalArithmeticOperations.sql.sql.outgolden files are unaffected because their schema lines usedf.schema.catalogString, which does not encode nullability. The codegen changes were additionally verified by inspecting generated bytecode: the deadisNullguards and stores for%///pmodover non-null inputs are gone under ANSI (LEGACY/TRY are unchanged).Was this patch authored or co-authored using generative AI tooling?
Generated-by: Isaac
This pull request and its description were written by Isaac.