search

apache / datafusion

Apache DataFusion SQL Query Engine
https://datafusion.apache.org/
Apache License 2.0
5.29k stars 973 forks source link

Convert internal representation of LogicalPlanBuilder from `LogicalPlan` to `Arc<LogicalPlan>` #10485

Open alamb opened 2 weeks ago

alamb commented 2 weeks ago 
          Great idea @ClSlaid

Thanks to @AbrarNitk we have a first version of LogicalPlanBuilder::from(arc_input) in https://github.com/apache/datafusion/pull/10466 🙏

Now that we have merged that PR we can make a second PR (and maybe a second ticket) about switching the internal representation from https://github.com/apache/datafusion/blob/8cc92a921f3b0296caf035edb66a4b3c422f084d/datafusion/expr/src/logical_plan/builder.rs#L98-L101

To 

 pub struct LogicalPlanBuilder { 
     plan: Arc<LogicalPlan>, 
 }

The rationale is as explained by @ClSlaid on https://github.com/apache/datafusion/issues/10465#issuecomment-2106192785 -- that in some cases this can prevent cloning the input

Originally posted by @alamb in https://github.com/apache/datafusion/issues/10465#issuecomment-2107222678

alamb commented 2 weeks ago

I think this is a good first issue as it is pretty clear what the ask is

iiiancampbell commented 2 weeks ago

take

alamb commented 2 weeks ago

Awesome -- thank you @iiiancampbell 🎉