[GitHub] [arrow] andygrove commented on a change in pull request #8097: ARROW-9821: [Rust][DataFusion] Support for User Defined ExtensionNodes in the LogicalPlan

[GitBox <gi...@apache.org>]

andygrove commented on a change in pull request #8097:
URL: https://github.com/apache/arrow/pull/8097#discussion_r484555403



##########
File path: rust/datafusion/src/execution/context.rs
##########
@@ -288,9 +288,17 @@ impl ExecutionContext {
 
     /// Optimize the logical plan by applying optimizer rules
     pub fn optimize(&self, plan: &LogicalPlan) -> Result<LogicalPlan> {
-        let plan = ProjectionPushDown::new().optimize(&plan)?;
-        let plan = FilterPushDown::new().optimize(&plan)?;
-        let plan = TypeCoercionRule::new(self).optimize(&plan)?;
+        let mut plan = ProjectionPushDown::new().optimize(&plan)?;
+        plan = FilterPushDown::new().optimize(&plan)?;
+        plan = TypeCoercionRule::new(self).optimize(&plan)?;
+
+        // apply any user supplied rules
+        let mut rules = self.state.config.optimizer_rule_source.rules();

Review comment:
       I don't have any objections to this approach, but I think I would have done it slightly differently. I am wondering if it makes sense to have one trait to cover the functionality that users can extend DataFusion with? Something like this ...
   
   ```rust
   trait UserDefinedQueryPlanner {
     fn optimize_logical_plan(plan: &dyn LogicalPlan) -> Result<Arc<dyn LogicalPlan>>;
     fn create_physical_plan(plan: &dyn LogicalPlan) -> Result<Arc<dyn PhysicalPlan>>;
   }
   ```
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org