[GitHub] [arrow] alamb commented on a change in pull request #9333: ARROW-11395: [DataFusion] Support custom optimizers

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

alamb commented on a change in pull request #9333:
URL: https://github.com/apache/arrow/pull/9333#discussion_r565652596



##########
File path: rust/datafusion/tests/user_defined_plan.rs
##########
@@ -178,7 +178,9 @@ async fn topk_plan() -> Result<()> {
 }
 
 fn make_topk_context() -> ExecutionContext {
-    let config = ExecutionConfig::new().with_query_planner(Arc::new(TopKQueryPlanner {}));
+    let config = ExecutionConfig::new()

Review comment:
       For other reviewers, here is a good example showing how the interface changes. Previously using custom optimizers meant you had to effectively implement a custom "query planner".
   
   I like it. 👍 

##########
File path: rust/datafusion/src/execution/context.rs
##########
@@ -324,19 +324,15 @@ impl ExecutionContext {
 
     /// Optimize the logical plan by applying optimizer rules
     pub fn optimize(&self, plan: &LogicalPlan) -> Result<LogicalPlan> {
-        // Apply standard rewrites and optimizations
+        let optimizers = &self.state.lock().unwrap().config.optimizers;
+
+        let mut new_plan = plan.clone();
         debug!("Logical plan:\n {:?}", plan);
-        let mut plan = ProjectionPushDown::new().optimize(&plan)?;
-        plan = FilterPushDown::new().optimize(&plan)?;
-        plan = HashBuildProbeOrder::new().optimize(&plan)?;
+        for optimizer in optimizers {
+            new_plan = optimizer.optimize(&new_plan)?;

Review comment:
       Eventually I would love to avoid this copying (aka this should ideally be `new_plan = optimizer.optimize(new_plan)` -- but we'll get there eventually :)




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