alamb commented on PR #15770:
URL: https://github.com/apache/datafusion/pull/15770#issuecomment-2971510953
> 1. Mutli-column order by not working. This seems like a bug / oversight.
Fundamentally I don't see any reason it shouldn't work, I'll have to
investigate why.
I suggest we file a ticket explaining what is going on so we don't lose the
context, but don't try and fix it in this PR
> 2. Interaction with `EnforceSorting` & co. After grokking the code for a
while I'm pretty convinced this is the only way to do things correctly.
`SortExec` needs to establish a link (via references) with the
`DataSourceExec`, and that needs to happen _before_ `EnforceSorting` runs
because it might modify the plan in the process. So we have no choice but to
teach `EnforceSorting` how to preserve this link. But that's the part I'm not
sure I did correctly since I don't fully understand how `EnforceSorting` works.
There may be other alternatives here. @berkaysynnada suggested that the right
rule ordering might "just work", but neither of us could get it to. We might be
able to split the filter pushdown into two steps: static (cannot make
assumptions about reference links but can modify the plan tree, e.g. for
`FilterExec`) and dynamic (can make reference links but cannot modify the plan
tree, e.g. `SortExec` and `HashJoin`). I think this would consist of adding a
new optimizer rule
`DynamicFilterPushdown` or something like that and maybe a
`Phase::{Dynamic,Static}` argument to the existing method on `ExecutionPlan`.
I just know we have also found EnforceSorting to be fragile (and there have
been quite a few bugs related to limits being lost, etc) so I agree with your
assesment that it is tough to know if it is correct.
I don't understand why `FilterPushdown` can't also push down DynamicFlters
if it was run after `EnforceSorting` but I vaguely remember it being discussed
and rejected before
> 3. Formatting in explain plans. I'm open to suggestions on this!
Likewise I think we should just file a follow on ticket. Maybe we can change
the `Display` impl to be just `<DynamicFilter>` or something to make it clear
it is something special (not just `false` constant)
--
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]
