You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2026/10/01 13:40:28 UTC
[PR] [SPARK-59922][SQL] Inline With definitions in filters inferred by InferFiltersFromConstraints [spark]
LuciferYang opened a new pull request, #59186:
URL: https://github.com/apache/spark/pull/59186
### What changes were proposed in this pull request?
`InferFiltersFromConstraints` plants redundant filters derived from a plan's constraints so they can be pushed down. When a constraint carries a `With` (a common-expression definition kept, for instance, inside a conditional branch by `RewriteWithExpression`, per SPARK-58902) whose definition reads only a column that is an equi-join key, the rule substitutes it through the key and plants a filter that still contains the `With` on the other side.
This adds `With.inlineDefinitions`, which rewrites a `With` into the expression it stands for, and uses it in `InferFiltersFromConstraints` to inline any `With` in a predicate the rule plants, at both of its planting sites. The inferred filter now holds the plain predicate instead of the `With`.
This also precedes https://github.com/apache/spark/pull/58845 (SPARK-59494), which lets a both-sides `With` survive in a join condition; with this in place that change does not have to touch `InferFiltersFromConstraints`.
### Why are the changes needed?
A `With` is opaque to filter translation: `DataSourceStrategy` cannot translate one, so a planted filter carrying a `With` is never pushed to the data source, losing the pushdown the inferred filter exists for. Inlining is safe here: only deterministic constraints are planted (both `inferNewFilter` and the constraint set filter on `deterministic`), and inlining a deterministic definition yields the same value at each reference as memoizing it once, so results are unchanged.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
New test in `InferFiltersFromConstraintsSuite`, `SPARK-59922: an inferred filter does not carry a With`: a conditional-branch `With` reading an equi-join key is inferred onto the other side, and the planted filter is asserted to carry no `With`. It fails without the inline and passes with it.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 4.8
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59922][SQL] Inline With definitions in filters inferred by InferFiltersFromConstraints [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59922][SQL] Inline With definitions in filters inferred by InferFiltersFromConstraints [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #59186:
URL: https://github.com/apache/spark/pull/59186#issuecomment-5936869552
Closing this. The premise was wrong: this is not a bug fix. A `With` has been evaluable since SPARK-58818, so an inferred filter that carries one already returns the correct rows. It evaluates the definition once per row rather than once per reference.
Inlining trades that for visibility to `ReplaceNullWithFalseInPredicate` and to V2 predicate translation, and in the common case the trade loses. The only `With` that reaches this rule is one `RewriteWithExpression` kept because its definition is referenced more than once and is not cheap, so inlining evaluates an expensive definition repeatedly. File sources gain nothing, since V1 translation has no case for the surrounding `CaseWhen` either way, and V2 cannot translate a UDF definition. On `CASE WHEN a > 0 THEN udf(a) BETWEEN 1 AND 100 ELSE false END` under an equi-join, the UDF calls in the inferred filter went from 18 to 27, with identical results. Gating the inlining on cheapness would not help: by construction, every `With` that reaches this rule fails that test.
I'll resolve SPARK-59922 as Won't Fix. Thanks @HyukjinKwon for the review.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59922][SQL] Inline With definitions in filters inferred by InferFiltersFromConstraints [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang closed pull request #59186: [SPARK-59922][SQL] Inline With definitions in filters inferred by InferFiltersFromConstraints
URL: https://github.com/apache/spark/pull/59186
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org