[SPARK-57027][SQL] SortMergeJoinExec: skip statically-dead branches in codegen#56075
[SPARK-57027][SQL] SortMergeJoinExec: skip statically-dead branches in codegen#56075gengliangwang wants to merge 3 commits into
Conversation
…n codegen
### What changes were proposed in this pull request?
Two statically-dead patterns in `SortMergeJoinExec` codegen:
1. `genComparison` emits `comp = 0; if (comp == 0) { comp = compare(k1); } ...`.
The first `if (comp == 0)` is always true (we just assigned 0). Emit
`comp = compare(k1);` directly; only wrap subsequent keys. `genComparison`
is called 5x per SMJ stage (twice in `genScanner`, three times in
`codegenFullOuter`). For single-key joins (common), each call collapses
to one line.
2. `genScanner` and `codegenFullOuter` emit
`if (k1IsNull || k2IsNull || ...) { handler }`. When all key `ExprValue`s
have `isNull == FalseLiteral`, the disjunction is statically `false` and
the whole block (including its `handleStreamedAnyNull` / "join with null
row" handler) is dead. Detect this and omit the block. Hits fact/
dimension joins on numeric keys where Spark has already proved
non-nullability.
### Why are the changes needed?
Smaller generated Java per SMJ stage. JIT eliminates the dead code at
runtime; the win is smaller generated source, more 64KB method-limit
headroom, and slightly faster Janino compile.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing test suites cover both paths with whole-stage codegen on and
off:
- `OuterJoinSuite` (SMJ full-outer codegen + interpreted scanner).
- `InnerJoinSuite` (SMJ codegen and non-codegen paths).
- `ExistenceJoinSuite` (SMJ existence path).
### Was this patch authored or co-authored using generative AI tooling?
Yes, with Claude Code.
Co-authored-by: Isaac
3f82673 to
89ee1bd
Compare
cloud-fan
left a comment
There was a problem hiding this comment.
Clean source-size cleanup in SortMergeJoinExec codegen.
Prior state. Two statically-dead patterns per SMJ stage: genComparison wrapped the first key compare in if (comp == 0) after comp = 0; (always true); genScanner / codegenFullOuter emitted if (k1IsNull || k2IsNull || ...) { handler } even when every ExprCode.isNull was FalseLiteral (always false). The Java compiler/JIT eliminates both, but the bloated source costs Janino parse time and 64 KB headroom.
Design. Detect the dead conditions at emit time. genComparison emits the first compare unguarded; only keys 2..n are wrapped. genScanner / codegenFullOuter gate the null-check block on exists(_.isNull != FalseLiteral). The check is sound: BoundReference.doGenCode sets isNull = FalseLiteral iff nullable=false, and UnaryExpression / BinaryExpression nullSafeCodeGen propagate that when their result is non-nullable. The PR is conservative (uses ExprCode-level isNull, not Expression-level nullable), so any divergence would be a missed optimization, not incorrect output.
Behavior equivalence verified for all-nullable, all-non-nullable, and mixed cases; the outer int comp = 0; declarations in both functions are preserved (load-bearing for Java definite-assignment).
One minor suggestion inline.
| val streamedKeyVars = createJoinKey(ctx, streamedRow, streamedKeys, streamedOutput) | ||
| val streamedKeysNullable = streamedKeyVars.exists(_.isNull != FalseLiteral) | ||
| val streamedAnyNull = streamedKeyVars.map(_.isNull).mkString(" || ") |
There was a problem hiding this comment.
When some keys are nullable and others aren't, streamedAnyNull still emits literal false terms in the disjunction — e.g. key1IsNull || false || key3IsNull. The Java compiler removes them, but since the stated goal is smaller emitted Java for Janino, filtering out FalseLiteral isNulls before joining would be cleaner:
| val streamedKeyVars = createJoinKey(ctx, streamedRow, streamedKeys, streamedOutput) | |
| val streamedKeysNullable = streamedKeyVars.exists(_.isNull != FalseLiteral) | |
| val streamedAnyNull = streamedKeyVars.map(_.isNull).mkString(" || ") | |
| val streamedKeyVars = createJoinKey(ctx, streamedRow, streamedKeys, streamedOutput) | |
| val nullableStreamedIsNulls = streamedKeyVars.map(_.isNull).filter(_ != FalseLiteral) | |
| val streamedKeysNullable = nullableStreamedIsNulls.nonEmpty | |
| val streamedAnyNull = nullableStreamedIsNulls.mkString(" || ") |
Same pattern applies to bufferedAnyNull (L232–233) and leftAnyNull / rightAnyNull in codegenFullOuter (L834–838).
There was a problem hiding this comment.
Done in 9ad99e16fd9. Both genScanner and codegenFullOuter now filter FalseLiteral isNull terms before joining the disjunction:
val nullableStreamedIsNulls = streamedKeyVars.map(_.isNull).filter(_ != FalseLiteral)
val streamedKeysNullable = nullableStreamedIsNulls.nonEmpty
val streamedAnyNull = nullableStreamedIsNulls.mkString(" || ")Mixed-nullability emit shrinks from e.g. key1IsNull || false || key3IsNull to key1IsNull || key3IsNull. All-nullable / all-non-nullable paths are unchanged. The all-non-nullable case still skips the check via streamedKeysNullable == false (same gate; just now derived from .nonEmpty).
Same pattern applied to leftAnyNull / rightAnyNull in codegenFullOuter.
lint-scala clean, 332/332 tests pass across SortMergeJoinSuite / OuterJoinSuite / ExistenceJoinSuite / InnerJoinSuite / WholeStageCodegenSuite.
… to drop dead false operands from the disjunction
| val firstCompare = pairs.headOption.map { case ((l, r), i) => | ||
| s"comp = ${ctx.genComp(leftKeys(i).dataType, l.value, r.value)};" | ||
| }.getOrElse("comp = 0;") | ||
| val restCompares = pairs.tail.map { case ((l, r), i) => |
There was a problem hiding this comment.
nit: pairs.tail throws UnsupportedOperationException on an empty sequence. SMJ guarantees at least one join key so this never fires, but drop(1) is the safe form:
| val restCompares = pairs.tail.map { case ((l, r), i) => | |
| val restCompares = pairs.drop(1).map { case ((l, r), i) => |
There was a problem hiding this comment.
Done in 689be7d0d87 — switched to pairs.drop(1). Thanks!
| | if ($streamedAnyNull) { | ||
| | $handleStreamedAnyNull | ||
| | } | ||
| | $checkStreamedAnyNull |
There was a problem hiding this comment.
nit: ${checkStreamedAnyNull.trim}`, line 383,960,963 should also use .trim.
There was a problem hiding this comment.
Done in 689be7d0d87 — added .trim to checkStreamedAnyNull (L366), checkBufferedAnyNull (L383), and checkLeftAnyNull/checkRightAnyNull in codegenFullOuter (L960/963).
…e anyNull check interpolations
- genComparison: pairs.drop(1) (safe on an empty seq) instead of pairs.tail.
- genScanner / codegenFullOuter: ${check*AnyNull.trim} so the emitted Java has no
blank-line padding when the null-check block is present (and nothing when it is empty).
Co-authored-by: Isaac
viirya
left a comment
There was a problem hiding this comment.
Took a close look at the codegen reasoning here — the change is correct and safe, and it's well-documented. A few notes.
The static-deadness detection is sound. The null check is dropped only when every key's isNull is exactly FalseLiteral, and that's the genuine non-nullable signal: createJoinKey sets ctx.currentVars = null, so bound keys go through BoundReference.doGenCode's row-access branch, which emits isNull = FalseLiteral precisely when the key is non-nullable. So if (anyNull) is omitted exactly when the condition is statically false — the branch is provably dead, with no behavioral change. Partial nullability is handled correctly too: with keys [nonNullable, nullable], the old false || isNull2 becomes isNull2 after filtering and the check is still emitted (streamedKeysNullable stays true). Filtering out the false operands in the kept-check case is a nice secondary cleanup.
The genComparison refactor preserves indices correctly — zipWithIndex runs before drop(1), so each surviving element keeps its original index and leftKeys(i).dataType still binds to the right key. No off-by-one, and the empty-keys case still collapses to comp = 0; as before.
Dropping the full-outer null-key branch is safe: outputLeftNoMatch / outputRightNoMatch are pure code-string fragments with no declarations, and they're still emitted at the other call sites in the join body, so removing their use inside the dead if loses nothing.
The two codegenFullOuter blocks mirror genScanner exactly in naming and structure, and the cross-referencing comment makes that intent clear.
Two optional nits, neither blocking:
nullableStreamedIsNullsreads slightly oddly — it's theisNullterms that survive filtering, not the nullable keys themselves. Something likeliveStreamedIsNullsmight read a touch clearer, but this is taste.- The dead branch being omitted for non-nullable keys isn't directly asserted by a test, but since it's a pure source-size optimization with no observable behavior (and there are no golden/string-match tests on the generated SMJ source), a dedicated test would be brittle and low-value. Fine to rely on the existing join suites.
LGTM.
What changes were proposed in this pull request?
This is a sub-task of SPARK-56908.
Two statically-dead patterns in
SortMergeJoinExeccodegen:genComparisonemitsThe first
if (comp == 0)is always true (we just assigned 0). Emitcomp = compare(k1);directly; only wrap subsequent keys.genComparisonis called 5x per SMJ stage (twice ingenScanner, three times incodegenFullOuter). For single-key joins (common), each call collapses to one line.genScannerandcodegenFullOuteremitif (k1IsNull || k2IsNull || ...) { handler }. When all keyExprValues haveisNull == FalseLiteral, the disjunction is staticallyfalseand the whole block (including itshandleStreamedAnyNull/ "join with null row" handler) is dead. Detect this and omit the block. Hits fact/dimension joins on numeric keys where Spark has already proved non-nullability.Why are the changes needed?
Smaller generated Java per SMJ stage. JIT eliminates the dead code at runtime; the win is smaller generated source, more 64KB method-limit headroom, and slightly faster Janino compile.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing test suites cover both paths with whole-stage codegen on and off:
OuterJoinSuite(SMJ full-outer codegen + interpreted scanner).InnerJoinSuite(SMJ codegen and non-codegen paths).ExistenceJoinSuite(SMJ existence path).Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code