Convert LogicalPlanBuilder to use Arc<LogicalPlan> #12040
Conversation
Summary Update struct to use Arc. Verified test passes. Used Arc::clone as much as I can; and unwrap_arc when a owned LogicalPlan is required. Keep pub fn input unchanged as LogicalPlan to limit change scope. If we change the pub fn, we can also get rid of this pattern: ``` unnest(unwrap_arc(self.plan), ...).map(Self::from) ```
| @@ -145,7 +150,7 @@ impl LogicalPlanBuilder { | |||
| coerce_plan_expr_for_schema(&recursive_term, self.plan.schema())?; | |||
| Ok(Self::from(LogicalPlan::RecursiveQuery(RecursiveQuery { | |||
| name, | |||
| static_term: Arc::new(self.plan.clone()), | |||
| static_term: Arc::clone(&self.plan), | |||
There was a problem hiding this comment.
This seems the only place a deep clone is "saved"?
There was a problem hiding this comment.
Another example in line 470.
project((*input).clone(), expr)
project(unwrap_arc(input), expr)
There was a problem hiding this comment.
Ah yes, here it might avoid the clone as well 👍
There was a problem hiding this comment.
FWIW I think we can avoid the clone by changing the signature from
pub fn to_recursive_query(
&self,to
pub fn to_recursive_query(
self,(the builder should be consumed)
There was a problem hiding this comment.
Another example in line 470.
I think since this is an associated function it could also be done indpendently to the change of the internal implementation of LogicalaPlanBuilder
|
Looked at the conflict. It can be resolved by changing |
|
I took the liberty of merging this PR up from main to resolve the conflict |
There was a problem hiding this comment.
Thank you @Dandandan and @jc4x4
I would be happy to merge this PR but I also think we could remove the clones in other ways that are smaller
However, in general, I think this PR improves the code / experience a bit so I think merging it is a good idea.
| @@ -145,7 +150,7 @@ impl LogicalPlanBuilder { | |||
| coerce_plan_expr_for_schema(&recursive_term, self.plan.schema())?; | |||
| Ok(Self::from(LogicalPlan::RecursiveQuery(RecursiveQuery { | |||
| name, | |||
| static_term: Arc::new(self.plan.clone()), | |||
| static_term: Arc::clone(&self.plan), | |||
There was a problem hiding this comment.
FWIW I think we can avoid the clone by changing the signature from
pub fn to_recursive_query(
&self,to
pub fn to_recursive_query(
self,(the builder should be consumed)
| @@ -145,7 +150,7 @@ impl LogicalPlanBuilder { | |||
| coerce_plan_expr_for_schema(&recursive_term, self.plan.schema())?; | |||
| Ok(Self::from(LogicalPlan::RecursiveQuery(RecursiveQuery { | |||
| name, | |||
| static_term: Arc::new(self.plan.clone()), | |||
| static_term: Arc::clone(&self.plan), | |||
There was a problem hiding this comment.
Another example in line 470.
I think since this is an associated function it could also be done indpendently to the change of the internal implementation of LogicalaPlanBuilder
Good catch. I've updated. |
|
Thanks again @jc4x4 |
Which issue does this PR close?
Closes #10485.
Rationale for this change
Refactor to avoid unnecessary copies of LogicalPlan.
What changes are included in this PR?
Update struct to use Arc. Verified test passes.
Used Arc::clone as much as I can; and unwrap_arc when a owned LogicalPlan is required.
Keep pub fn input unchanged as LogicalPlan to limit change scope. If we change the pub fn, we can also get rid of this pattern like below. If this broader change is wanted, I can follow up with a separate PR.
Are these changes tested?
Run unit tests inside builder.rs
Are there any user-facing changes?
No. Pub fn is explicitly not affected.