timsaucer commented on issue #23678: URL: https://github.com/apache/datafusion/issues/23678#issuecomment-5016212980
I've been working with a few agents this morning and I've settled on a plan that I think is reasonable: ### 1. Catalog contract extraction - Move the catalog/provider trait closure into `datafusion-session`. - Reexport it from `datafusion-catalog`. - Add `Session::catalog_list` (`fn catalog_list(&self) -> Arc<dyn CatalogProviderList>;`) - Implement it for `SessionState`. - Preserve existing public import paths. This can be reviewed independently from planner behavior. ### 2. Physical optimizer interface extraction - Move `PhysicalOptimizerRule` and `PhysicalOptimizerContext` definitions. - Reexport them from `datafusion-physical-optimizer`. - Keep all optimizer implementations where they are. - Establish the minimal planning capabilities required from `Session`. (`fn query_planner(&self) -> Arc<dyn QueryPlanner>;` and `fn optimize_logical_plan(&self, plan: &LogicalPlan) -> Result<LogicalPlan>;`) ### 3. Planner contract extraction and signature conversion After #23649 merges: - Move `QueryPlanner`, `PhysicalPlanner`, and `ExtensionPlanner` definitions. - Change every session argument to `&dyn Session`. - Add `Session::query_planner`. - Add the logical-optimization capability needed for nested plans. - Update `DefaultQueryPlanner` and `DefaultPhysicalPlanner`. - Keep default implementations in core. ### 4. Prove implementation independence Add a test session implementation that is deliberately **not** `SessionState` and cannot be downcast to it. Demonstrate that it can: - expose and use a catalog; - expose a custom query planner; - run the planner through `&dyn Session`; - plan extension nodes and physical expressions without downcasting. This is more valuable than tests that only substitute `&dyn Session` while still passing a `SessionState`. ### 5. FFI work as a subsequent change Once the Rust contracts settle: - extend the FFI session wrapper with catalog and query-planner access; - add query/physical planner wrappers; - test across the actual `cdylib` boundary; -- 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]
