milenkovicm commented on PR #18739:
URL: https://github.com/apache/datafusion/pull/18739#issuecomment-3606422048
> > Is the physical optimiser rule in the right place for such
functionality, or would it break separation of concerns and make optimiser do
too many things ?
>
> I'd say so yeah. It's during the optimization where dynamic filters are
created, which leads me to think that if their state was to be registered in a
global place for outside access, it's at that moment when it should happen.
Could this be accomplished providing your own query planner? Or keeping
state in your own optimiser rule which, rules are "arced" so you could keep
reference to it and access it as needed?
>
> > Would you be able to provide more details what do you mean by "more
elegant solution", I guess if there is PR description we would be able to
understand it from there
>
> There's still a lot of things I should flesh out before giving a proper
answer to this, but TL;DR is the second solution exposed in #18296.
That is my feeling as well, this PR is merged without all the things are
flashed out.
>
> > Was there a genuine need for such a rush that the PR was merged without
any associated issue or description?
>
> I would not say there was a rush, the original PR #18168 has been out
there for more than month ago. It's true that the context is a bit scattered
across PR comments rather than being properly centralized in one issue.
>
yes, comments do not help understanding the context, and problem breakdown
in sense of PR/Issue description helps everybody.
> For @adriangb and I this change felt natural as this is something we've
been discussing publicly in the last months, but we might be biased as we were
the ones discussing,
An external perspective can be invaluable in developing a more effective
design.
I personally believe introduction of `OptimizerContext` nor change of
`Extensions` bring any benefits to ballista at this point. None of them have
any strong reason to be introduced.
Switching to accept SessionConfig somewhat might make sense. However, I’m
wondering if the optimiser rule can retain the state needed to extract from the
SessionConfig extension. Since both are tied to SessionContext, I don’t see a
strong reason to keep it in two places.
> so if you think this needs more thought we are on time to revert whatever
part of this PR you think needs more discussion.
I would leave up to you to make such decision, you're the one approving the
PR; I personally struggle to see value of this PR at this moment.
--
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]
