adriangb commented on code in PR #15770:
URL: https://github.com/apache/datafusion/pull/15770#discussion_r2051065573
##########
datafusion/physical-optimizer/src/push_down_filter.rs:
##########
@@ -382,7 +383,7 @@ impl PhysicalOptimizerRule for PushdownFilter {
context
.transform_up(|node| {
- if node.plan.as_any().downcast_ref::<FilterExec>().is_some() {
+ if node.plan.as_any().downcast_ref::<FilterExec>().is_some()
|| node.plan.as_any().downcast_ref::<SortExec>().is_some() {
Review Comment:
@berkaysynnada I didn't notice this in the original PR. This seems
problematic. IMO doing downcast matching here is a smell that the API needs
changing. It limits implementations to a hardcoded list of plans, which defeats
the purpose of making DataFusion pluggable / having a `dyn ExecutionPlan`. The
original implementation didn't require this. I think this goes hand-in hand
with the `revisit` parameter. It seems that you were able to get from 3 methods
down to 2 by replacing one of them with this downcast matching and the other
with the extra recursion via the `revisit` parameter. It would be great to
iterate on this and find a way to avoid the downcast matching.
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
