Copilot commented on code in PR #58377:
URL: https://github.com/apache/spark/pull/58377#discussion_r3878794931
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RewriteWithExpression.scala:
##########
@@ -186,6 +186,8 @@ object RewriteWithExpression extends Rule[LogicalPlan] {
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.
Review Comment:
The PR description states SPARK-58902 is fixed by introducing lazy per-row
memoization for multi-referenced common expressions in conditional branches,
but this code path still unconditionally inlines `With` (and only adds a note).
This does not change semantics and still permits multiple evaluations of
nondeterministic expressions per row; either implement the described lazy
memoization rewrite here (and for the join-condition TODO if in scope) or
update the PR title/description to match what’s actually being delivered.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/RewriteWithExpressionSuite.scala:
##########
@@ -500,4 +500,18 @@ class RewriteWithExpressionSuite extends PlanTest {
val plan = testRelation.select(expr.as("col"))
comparePlans(Optimizer.execute(plan), testRelation.select((a + a +
1).as("col")))
}
+
+ 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)
+ }
Review Comment:
This new SPARK-58902 test doesn’t assert the intended single-evaluation
behavior (it only checks `optimized.output.length`), so it would pass even if
conditional-branch `With` is still inlined and nondeterministic expressions are
duplicated. Strengthen it to use a nondeterministic common expression (e.g.,
`Rand`) and assert the optimized plan contains only one instance of that
nondeterministic expression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
