szehon-ho commented on code in PR #57727:
URL: https://github.com/apache/spark/pull/57727#discussion_r3752500869
##########
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:
Both claims were wrong, thanks. I rewrote the comment, though I ended up
somewhere different from the suggestion, because working through it showed the
comment was explaining the wrong thing entirely.
One correction to the reason first. `ExecScalarSubquery` and
`InSubqueryExec` are not unconditionally deterministic:
`PlanExpression.deterministic` is `children.forall(_.deterministic) &&
plan.deterministic` (`catalyst/.../expressions/subquery.scala:47-48`), and
`plan` there is the `BaseSubqueryExec`, whose `QueryPlan.deterministic` folds
in its own expressions and children (`QueryPlan.scala:148-151`).
`ScalarSubquery` is `LeafLike`, so its determinism is exactly its physical
subquery plan's, not `true`. The exec form therefore is not strictly more
permissive on that clause; it asks the same question of the planned form of the
same plan. Your conclusion holds either way, just by the two agreeing rather
than by one being weaker. You are right that `includeSubquery` is inert at this
call site.
The bigger problem is that both versions of the comment justified the screen
by pointing at the strategy, which gets the dependency backwards. A screen here
can only shrink the pushed set, so it cannot be what protects a filter whose
`FilterExec` was already deleted. The screen exists for the ordinary case: a
runtime filter is evaluated twice, by the source for pruning and by the
`FilterExec`, and the two have to agree. `isPushablePartitionFilter`'s own
scaladoc says exactly this ("whether the data source can be trusted to evaluate
`f` in place of Spark"). The strategy consults it *because* this screen can
decline to push, so a filter this screen would decline must keep its
`FilterExec`.
So the comment now leads with double evaluation, then derives the coupling
from it, and closes with the note that both sites accept everything today since
the strategy already keeps non-deterministic filters out of `runtimeFilters`
(SPARK-58207). No claim about forms is needed, which also removes the sentence
we have now been wrong about twice.
--
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]
