adriangb commented on code in PR #19446:
URL: https://github.com/apache/datafusion/pull/19446#discussion_r2644629251
##########
datafusion/sqllogictest/test_files/dynamic_filter_pushdown_config.slt:
##########
@@ -752,3 +752,577 @@ DROP TABLE sorted_parquet;
statement ok
SET datafusion.optimizer.enable_sort_pushdown = true;
+
+
+# Test 7: Sort pushdown with constant column filtering
Review Comment:
Why in `dynamic_filter_pushdown_config.slt`?
##########
datafusion/physical-plan/src/filter.rs:
##########
@@ -603,6 +604,27 @@ impl ExecutionPlan for FilterExec {
})
}
+ fn try_pushdown_sort(
Review Comment:
A big picture question on this one: is it beneficial to push down sort order
past a filter? If it's "free" of course. But if there's runtime cost to
re-ordering rows, it may be cheaper to filter then re-order (if the filter is
very selective).
I think in practice, for Parquet, everything ends up happening in the scan
itself and filters always get applied first, so this is inconsequential. But in
theory it could hurt data sources that e.g. don't support filter pushdown and
hence rely on a FilterExec.
##########
datafusion/physical-plan/src/coop.rs:
##########
@@ -328,6 +329,27 @@ impl ExecutionPlan for CooperativeExec {
) -> Result<FilterPushdownPropagation<Arc<dyn ExecutionPlan>>> {
Ok(FilterPushdownPropagation::if_all(child_pushdown_result))
}
+
+ fn try_pushdown_sort(
Review Comment:
This seems like a good improvement but is orthogonal to the change to use
`EqProperties` (just noting)
--
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]
