peter-toth commented on code in PR #57727:
URL: https://github.com/apache/spark/pull/57727#discussion_r3734845356
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/PushDownUtils.scala:
##########
@@ -218,6 +222,26 @@ object PushDownUtils extends Logging {
}
translatedFiltersPushed || partPredicatesPushed
+
+ case catalystScan: SupportsRuntimeCatalystFiltering if
runtimeFilters.nonEmpty =>
+ // A DPP filter degrades to TrueLiteral when its subquery is pruned
away; it carries no
+ // information for the source. The V2 path above drops these
implicitly because
+ // translateRuntimeFilterV2 returns None; here we push Catalyst
expressions directly,
+ // so filter them out explicitly.
+ // Screen with the same pushability guard as the V2 PartitionPredicate
path
+ // (deterministic, no subquery, no Python UDF). Keeps
non-deterministic filters
+ // from being the sole evaluator when fullyPushedFilterAttributes
drops FilterExec.
Review Comment:
**Finding 13.** The guard moved exactly as suggested and the non-determinism
clause reads correctly now. The two claims that replaced it don't, and one of
them is why the guard looks load-bearing when it isn't.
*"the same guard, applied to the same form"* — the forms still differ.
`DataSourceV2Strategy` screens the *logical* filter, where the subquery is a
Catalyst `ScalarSubquery`; `pushRuntimeFilters` runs at execution time, after
`PlanSubqueries`, so it sees `ExecScalarSubquery` / `InSubqueryExec`. Screening
before the unwrap keeps the DPP wrapper on, but it doesn't make the two sides
look at the same expression.
*"what the guard rejects here is a residual subquery or a Python UDF"* — a
residual subquery is the one thing it cannot reject. `includeSubquery = true`
disables that clause, and it would be inert regardless:
`SubqueryExpression.hasSubquery` matches `SubqueryExpression`, while
`ExecSubqueryExpression extends PlanExpression[BaseSubqueryExec]` is its
*sibling*, not its subclass (`catalyst/.../expressions/subquery.scala:76` vs
`core/.../execution/subquery.scala:36`). So at this call site `includeSubquery`
is a no-op and the guard is exactly `deterministic && no PythonUDF` — and per
your own reply neither can reach here today.
The invariant you want is still true, and for a stronger reason than
sameness of form: the exec-form guard is strictly *weaker* than the
logical-form one. `ExecScalarSubquery` and `InSubqueryExec` have no children,
so `deterministic` is true whatever the subquery contained, and `hasSubquery`
is false on either form. A filter the strategy dropped from `postScanFilters`
therefore always passes here — the direction that matters can't fail — while
the reverse (kept there, pushed here) is harmless double evaluation. Worth
saying that instead, since it's the property a future edit could break:
```scala
// Screen with the same predicate DataSourceV2Strategy applies
before dropping a fully
// pushed filter from postScanFilters, so a filter dropped there is
never rejected here.
// The two see different forms -- logical there, post-PlanSubqueries
here -- but the exec
// form is strictly more permissive (ExecScalarSubquery /
InSubqueryExec have no children,
// so they are deterministic and are not SubqueryExpressions), so
that direction holds.
// Nothing non-deterministic or Python-UDF-bearing reaches this line
today, so the screen
// is drift protection.
```
--
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]
