adriangb commented on PR #19446:
URL: https://github.com/apache/datafusion/pull/19446#issuecomment-3691551221
> > I'm comfortable with `ProjectionExec` and `CooperativeExec` but I think
`FilterExec` needs some more discussion since pushing ordering below filters
might have a negative impact on performance for some queries / file sources.
>
> @adriangb Happy holidays! Thank you for the feedback! I completely
understand your concern about FilterExec.
>
> If i makes sense right, i've updated the implementation to address the
performance issue you mentioned. Now `FilterExec::try_pushdown_sort` only
pushes down the sort when the child node already has an output ordering:
>
> ```rust
> fn try_pushdown_sort(
> &self,
> order: &[PhysicalSortExpr],
> ) -> Result<SortOrderPushdownResult<Arc<dyn ExecutionPlan>>> {
> if let Some(_child_ordering) = self.input.output_ordering() {
> match self.input.try_pushdown_sort(order)? {
> SortOrderPushdownResult::Exact { inner } => {
> let new_exec =
Arc::new(self.clone()).with_new_children(vec![inner])?;
> Ok(SortOrderPushdownResult::Exact { inner: new_exec })
> }
> SortOrderPushdownResult::Inexact { inner } => {
> let new_exec =
Arc::new(self.clone()).with_new_children(vec![inner])?;
> Ok(SortOrderPushdownResult::Inexact { inner: new_exec
})
> }
> SortOrderPushdownResult::Unsupported => {
> Ok(SortOrderPushdownResult::Unsupported)
> }
> }
> } else {
> Ok(SortOrderPushdownResult::Unsupported)
> }
> }
> ```
>
> This way:
>
> * When the data source can optimize to use pushdown sort, so we
pushdown to leverage the existing ordering.
>
> * When the data source is unsorted, we don't pushdown, so sorting
happens after filtering on the smaller filtered dataset
>
>
> This should avoid the performance degradation you were concerned about
while still enabling the optimization for sorted data sources. The new tests
validate this with the `WHERE timeframe = 'quarterly'` cases, which require
pushdown through FilterExec to utilize the sorted Parquet file's ordering.
>
> What do you think about this approach?
That's a super interesting and smart approach! It absolutely makes sense and
nukes the problem for cases where the sort orders match (i.e. you basically
know the sort will be eliminated). But I feel for those cases `EnforceSorting`
optimizer should already handle. For other cases it's still not clear if it
always makes sense. Consider a DataSource that does accept sort pushdown (at a
cost e.g. buffering in memory) but doesn't accept filter pushdown. How do we
know when it makes sense to push down the sort?
This issue of not pushing things down past filters seems to have come up for
both projection and sort pushdown. In both cases we are in a situation of
"always push it down into the DataSource if possible, otherwise it depends on
costs and we don't have a cost based evaluator so hard to say". I do think it's
worth asking ourselves the question: why are there some `FilterExec`s left?
Could we re-order the optimizer rules so that the filters always get pushed
down first? Do we need to implement filter pushdown for more operators? Maybe
you could share some of the scenarios / queries where the `FilterExec` is
preventing sort pushdown?
I still think our best bet would be to remove that once piece from this PR
so that we can merge the rest then we can continue to discuss the `FilterExec`
case.
--
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]
