[SPARK-57216][SQL] Skip dead null-check branches in NaNvl codegen for non-nullable children#56276
[SPARK-57216][SQL] Skip dead null-check branches in NaNvl codegen for non-nullable children#56276LuciferYang wants to merge 2 commits into
Conversation
… non-nullable children
NaNvl.doGenCode wrapped each child in a null check: an outer if (leftGen.isNull) {...} else {...} and, in the NaN-fallback path, an inner if (rightGen.isNull) {...} else { ev.value = rightGen.value; }. For a non-nullable child the isNull is statically false, so drop the outer wrapper when left is non-nullable and the inner one when right is non-nullable, emitting just the value assignment. The NaN check and lazy right-child evaluation are preserved. The body is refactored into small Block values to express the gating cleanly. Follows the non-nullable-children dead-branch pattern of GenArrayData.genCodeToCreateArrayData / CreateNamedStruct; shrinks the generated method per non-nullable child. Adds checkEvaluation cases with nullable BoundReference children to cover the both-children-nullable codegen path.
Part of SPARK-56908.
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
+1, LGTM. Thank you, @LuciferYang .
|
Flagging against the scope guidance added to the umbrella SPARK-56908. This is a source-only dead-branch elimination: Janino constant-folds Before merging, please confirm this removes more than source text (a |
|
Close this PR after checking against the preceding criteria. |
3 participants
What changes were proposed in this pull request?
NaNvl.doGenCodealways wrapped each child in a null check: an outerif (leftGen.isNull) { ev.isNull = true; } else { ... }, and (in the NaN-fallback path) an innerif (rightGen.isNull) { ev.isNull = true; } else { ev.value = rightGen.value; }. For a non-nullable child theisNullis staticallyfalse, so the wrapper is dead.This drops the outer wrapper when
leftis non-nullable and the inner one whenrightis non-nullable, emitting just the value assignment. The NaN check and the lazy evaluation of the right child (evaluated only whenleftis non-null and NaN) are preserved. The body is refactored into a few smallBlockvalues to express the gating cleanly.Follows the SPARK-56908 non-nullable-children dead-branch pattern of
GenArrayData.genCodeToCreateArrayData/CreateNamedStruct.Why are the changes needed?
It removes a dead comparison/branch per non-nullable child, shrinking the generated method (~3 lines per non-nullable child, ~6 when both are non-nullable), which helps with the JVM 64KB method / constant-pool limits, Janino compile time, and JIT work. Part of SPARK-56908.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing
NullExpressionsSuite"nanvl" andColumnExpressionSuite"nanvl", plus addedcheckEvaluationcases with nullableBoundReferencechildren that exercise the both-children-nullable codegen path (the existing non-nullable-literal cases now take the dead-branch-skipping paths instead).checkEvaluationruns the interpreted and codegen paths; behavior is unchanged.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)