alamb commented on code in PR #12040:
URL: https://github.com/apache/datafusion/pull/12040#discussion_r1720754539
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -385,7 +390,7 @@ impl LogicalPlanBuilder {
Ok(Self::from(LogicalPlan::Prepare(Prepare {
name,
data_types,
- input: Arc::new(self.plan),
+ input: Arc::clone(&self.plan),
Review Comment:
Since this method takes `self` by value, I don't think you need this clone
here (or the other methods that take `self`:
```suggestion
input: self.plan,
```
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -93,12 +93,17 @@ pub const UNNAMED_TABLE: &str = "?table?";
/// ```
#[derive(Debug, Clone)]
pub struct LogicalPlanBuilder {
- plan: LogicalPlan,
+ plan: Arc<LogicalPlan>,
}
impl LogicalPlanBuilder {
/// Create a builder from an existing plan
pub fn from(plan: LogicalPlan) -> Self {
+ Self { plan: Arc::new(plan) }
Review Comment:
A more "idomatic" way to do this would be call these methods
```rust
impl LogicalPlanBuilder {
pub fn new(plan: LogicalPlan) -> Self {...}
pub fn new_from_arc(plan: Arc<LogicalPlan>) -> Self { ... }
...
}
```
And then implement `From` which would allow using `into()` syntax
```rust
impl From<LogicalPlan> for LogicalPlanBuilder {
}
```
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
