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:
[email protected]
