timsaucer commented on code in PR #23622:
URL: https://github.com/apache/datafusion/pull/23622#discussion_r3596528489
##########
datafusion/core/src/physical_planner.rs:
##########
Review Comment:
It's worse than that. `Clone` makes it no longer `dyn` compatible, as would
making a method that returns `-> Self`.
One agent was suggesting adding a wrapper
```
struct SessionWithProps<'a> {
inner: &'a dyn Session,
execution_props: ExecutionProps,
}
```
and then passing that everywhere, but this seems like a terrible approach to
me. You'd implement `Session` on it and delegate almost all of the methods to
`inner` except the `execution_props`. That would be the way to keep a minimal
code change, but honestly it feels like it's adding a ton of technical debt.
I think our options are:
- This PR merges into yours and then yours into main. It's a small-ish
change and targeted with minimal/no churn on other projects. The big down side
is it does create some brittleness to the code.
- Add in the wrapper type suggested above, send it around everywhere, but
again that feels like it's just adding technical debt.
- Go back to my original, large PR. It does need a correction for this user
defined expression issue, but I can work on that.
--
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]
