alamb commented on issue #9637: URL: https://github.com/apache/arrow-datafusion/issues/9637#issuecomment-2012208811
With some somewhat hacky code in https://github.com/apache/arrow-datafusion/pull/9708#issuecomment-2012196911 I saw a 25% performance improvement. Given the results I saw in https://github.com/apache/arrow-datafusion/pull/9708 here is my proposal: 1. Update the Optimizer implementation / Optimizer APIs to rewrite existing LogicalPlans rather than make new ones 2. Add the necessary APIs to LogicalPlan 3. Contemplate eventually switching to use `Box<LogicalPlan>` instead of `Arc<LogicalPlan>` to store children in LogicalPlan.... I am not sure which of these APIs would be better (rewrite in place via `&mut plan` or take ownership (via `plan`)). I feel like the first one would likely be faster (as it avoids copying at all), but the second is easier to reason about 🤔 ```rust impl OptimizerRule { ... /// does this rule support rewriting plans rather than copying them? fn supports_mut (&self) -> bool { return false } /// if supports_mut, returns true, rewrites `plan` in place fn optimize_mut( &self, plan: &mut LogicalPlan, config: &dyn OptimizerConfig ) -> Result<Transformed<()>, DataFusionError> { not_implemented_err!("try_optimized_owned is not implemented for this rule") } ... ``` ```rust impl OptimizerRule { ... /// does this rule support rewriting owned plans? fn supports_owned(&self) -> bool { return false } /// if supports_owned returns true, rewrites the LogicalPlan in returning a newly rewritten plan fn try_optimize_owned( &self, plan: LogicalPlan, config: &dyn OptimizerConfig ) -> Result<Transformed<LogicalPlan>, DataFusionError> { not_implemented_err!("try_optimized_owned is not implemented for this rule") } ... ``` -- 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]
