gabotechs commented on code in PR #23622: URL: https://github.com/apache/datafusion/pull/23622#discussion_r3596097231
########## datafusion/core/src/physical_planner.rs: ########## Review Comment: I'm having some second thoughts about this approach... and realizing that it might actually be better to go with your original PR, even if it's a bigger change, it should be more future-proof (https://github.com/apache/datafusion/pull/22340) ########## datafusion/core/src/physical_planner.rs: ########## Review Comment: This might actually be brittle... we are passing contradicting information to this function: 1. The `ExecutionProps` present in the `planner` 2. The `ExecutionProps` present in the `session_state` Both contain different info (one has uncorrelated subquery information, and the other doesn't). This `.plan_extension` method will call people's code, and I imagine this has chances of blowing up in weird ways in their faces. -- 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]
