HyukjinKwon commented on code in PR #59186:
URL: https://github.com/apache/spark/pull/59186#discussion_r4156480875
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/InferFiltersFromConstraintsSuite.scala:
##########
@@ -465,4 +465,39 @@ class InferFiltersFromConstraintsSuite extends PlanTest {
comparePlans(optimized, correctAnswer)
}
+ test("SPARK-59922: an inferred filter does not carry a With") {
+ val x = testRelation.subquery("x")
+ val y = testRelation.subquery("y")
+
+ // The left user filter keeps a With inside a conditional branch. The
branch references
+ // only x.a, an equi-join key, so InferFiltersFromConstraints substitutes
it through the
+ // key and plants a matching filter on the right (over y.a). A With is
opaque to filter
+ // pushdown, so the inferred filter must not carry one -- the user's own
filter still may.
+ val xa = x.output.head
+ val branchWith = With(xa + xa) { case Seq(r) => (r > 1) && (r < 10) }
+ val leftPredicate = CaseWhen(Seq((xa > 0) -> branchWith),
Some(Literal(false)))
+
+ val originalQuery = x.where(leftPredicate)
+ .join(y, Inner, Some("x.a".attr === "y.a".attr))
+ .analyze
+
+ // The right relation's equi-join key; exprIds are stable through
optimization.
+ val ya = originalQuery.collectFirst { case j: Join => j.right.output.head }
+ .getOrElse(fail("expected a Join in the analyzed plan"))
+
+ val optimized = Optimize.execute(originalQuery)
+
+ // Conditions over the right side that carry the propagated CaseWhen
branch predicate.
+ val inferred = optimized.collect {
+ case Filter(cond, _) if cond.references.contains(ya) => cond
+ }.filter(_.exists(_.isInstanceOf[CaseWhen]))
+
+ // PREMISE: the branch predicate was inferred and planted on the right
side.
+ assert(inferred.nonEmpty, s"no inferred branch filter over the right
side:\n$optimized")
+
+ // The inferred filter must not carry a With.
+ assert(inferred.forall(cond => !cond.exists(_.isInstanceOf[With])),
Review Comment:
**Nit (P3):** This assertion only checks that no `With` node survives. If
`inlineDefinitions` returned `withExpr.child` unchanged, the right-side filter
would still contain a `CaseWhen` with no `With`, just with dangling
`CommonExpressionRef`s, and the test would pass even though the plan can't be
evaluated. The test also only drives the `inferNewFilter` path. Nothing covers
the Filter-node site (`Filter(And(inlineWiths(newFilters.reduce(And)),
condition), child)`), which is reached by e.g. `x.a = x.b AND CASE WHEN x.a > 0
THEN With(x.a + x.a)(...) END`, or a nested `With` whose inner child references
an enclosing definition.
Could you compare against the expected inlined predicate with `comparePlans`
(as the rest of this suite does), or at least assert that no
`CommonExpressionRef` remains? It would also help to add a Filter-node case and
a nested-`With` case.
**Verification:**
- **Regression:** The join-side case checks the exact inlined predicate (or
the absence of any CommonExpressionRef), so a dangling-ref substitution fails.
- **Behavior:** A single-filter case shows the inferred, substituted
predicate planted by the Filter-node site is inlined.
- **Behavior:** A nested-With case shows inner and enclosing definitions are
both substituted in the planted predicate.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -2303,9 +2303,30 @@ object InferFiltersFromConstraints extends
Rule[LogicalPlan]
if (newPredicates.isEmpty) {
plan
} else {
- Filter(newPredicates.reduce(And), plan)
+ Filter(inlineWiths(newPredicates.reduce(And)), plan)
}
}
+
+ /**
+ * Inlines any `With` in an inferred predicate. `RewriteWithExpression`
keeps a `With` where it
+ * cannot pre-evaluate the definition -- a conditional branch, for one --
and such a `With` can
+ * reach a constraint and be carried, through an equi-join key, into a
filter inferred for the
+ * other side. A `With` is opaque to filter pushdown (`DataSourceStrategy`
cannot translate one),
Review Comment:
**Nit (P3):** `DataSourceStrategy` is the wrong consumer to name here. The
only `With` that reaches this point sits inside a conditional branch, so even
after inlining, the predicate still contains a `CaseWhen`/`If`, and
`DataSourceStrategy.translateFilter` has no case for those. V1/file-source
pushdown therefore isn't unblocked. The consumer that actually benefits is V2
predicate translation: `V2ExpressionBuilder` translates `CaseWhen` but has no
case for `With`. Could you reword this to name V2 predicate pushdown, so
readers don't infer a V1 benefit?
**Verification:**
- **Inspection:** The comment's stated consumer agrees with the translation
code: V2ExpressionBuilder translates CaseWhen, and DataSourceStrategy has no
CaseWhen/If translation.
--
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]
