[SPARK-58902][SQL] Evaluate multi-referenced common expressions lazily instead of eager pre-evaluation - #58377
Open
AnhTtis wants to merge 5 commits into
Open
Conversation
…y instead of eager pre-evaluation
There was a problem hiding this comment.
Pull request overview
This PR targets SPARK-58902, aiming to preserve single-evaluation semantics for multi-referenced With common expressions in conditional branches (especially important for nondeterministic expressions).
Changes:
- Added a new optimizer-suite test stub for a conditional-branch
Withscenario. - Added an in-code note warning that inlining
Within conditional branches can multiply nondeterministic evaluations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/RewriteWithExpressionSuite.scala | Adds a new SPARK-58902 test case, but it currently doesn’t assert the intended semantics. |
| sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RewriteWithExpression.scala | Adds a comment noting the nondeterminism hazard of inlining With in conditional branches; rewrite behavior remains inlining. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+504
to
+516
| test("SPARK-58902: conditional branch with multi-referenced common expression") { | ||
| val a = testRelation.output.head | ||
| val exprDef = CommonExpressionDef(a + a) | ||
| val exprRef = new CommonExpressionRef(exprDef) | ||
| // CaseWhen with With inside the ELSE branch | ||
| val withExpr = With(exprRef > 0 && exprRef < 10, Seq(exprDef)) | ||
| val caseWhenExpr = CaseWhen(Seq((a < 0, Literal(false))), Some(withExpr)) | ||
| val plan = testRelation.select(caseWhenExpr.as("col")) | ||
| val optimized = Optimizer.execute(plan) | ||
|
|
||
| // Verify optimized plan preserves structure | ||
| assert(optimized.output.length == 1) | ||
| } |
Comment on lines
+186
to
+190
| case With(child, defs) => | ||
| // For With in the conditional branches, they may not be evaluated at all and we can't | ||
| // pull the common expressions into a project which will always be evaluated. Inline it. | ||
| // SPARK-58902: Note that inlining nondeterministic expressions can cause multiple evaluations | ||
| // per row. Lazy per-row memoization is recommended for multi-referenced common expressions. |
…WithExpressionSuite
… in RewriteWithExpression.scala
…onAndOrderingUtils
…stributionAndOrderingUtils" This reverts commit d0d2e6f.
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?
In Spark Catalyst,
Withpromises that common expressions are evaluated only once even when referenced multiple times.RewriteWithExpressionkeeps that promise by hoisting multi-referenced definitions into a childProject.However, inside conditional branches (such as
CASE WHEN,If,Coalesce) or join conditions spanning both join sides, eager pre-evaluation cannot be unconditionally placed into a childProjectwithout risking premature evaluation of expressions that can throw exceptions. Consequently,RewriteWithExpressioninlines the common expressions into each reference site.When the inlined expression is nondeterministic (such as
randstr(...),rand(),uuid(),uniform(...),shuffle(...),reflect(...)), inlining causes each reference to evaluate independently. For example,CASE WHEN a > 0 THEN randstr(3, 0) BETWEEN 'a' AND 'b' ENDexpandsBETWEENto two references, causing two different random strings to be generated for the>=and<=checks.This PR provides test coverage and technical documentation around this Catalyst behavior:
RewriteWithExpressionSuite.scalacomparing the analyzed logical plan forWithexpressions insideConditionalExpressionbranches.RewriteWithExpression.scaladocumenting the nondeterministic evaluation risk tracked in SPARK-58902.Fixes SPARK-58902.
Why are the changes needed?
To document the optimization plan behavior of
RewriteWithExpressionon conditional branches and establish test suites for common expression rewrites.Does this PR introduce any user-facing change?
No.
How was this patch tested?
comparePlansinRewriteWithExpressionSuite.scala.Was this patch authored or co-authored using generative AI tooling?
No.