Deprecate duplicate function LogicalPlan::with_new_inputs
#8707
Conversation
github-actions
bot
added
logical-expr
| /// Note: sometimes [`Self::with_new_exprs`] will use schema of | ||
| /// original plan, it will not change the scheam. Such as | ||
| /// `Projection/Aggregate/Window` |
There was a problem hiding this comment.
This comment looks incorrect. For Projection/Aggregate/Window, the schema might change if input/expressions are changed.
| assert_eq!(window_expr.len(), expr.len()); | ||
| Ok(LogicalPlan::Window(Window { | ||
| input: Arc::new(inputs[0].clone()), | ||
| window_expr: expr, | ||
| schema: schema.clone(), | ||
| })) |
There was a problem hiding this comment.
This looks like a potential bug. Since input/window_expr will change, we shouldn't reuse previous schema.
There was a problem hiding this comment.
maybe @mustafasrepo remembers the context -- I vaguely remember something like this but can't remember the specifics
There was a problem hiding this comment.
This looks like a potential bug. Since input/window_expr will change, we shouldn't reuse previous schema.
Agree that, previous version wasn't robust to expression and input changes. This version is much better. Thanks @viirya for catching this.
| @@ -540,38 +540,6 @@ impl LogicalPlan { | |||
| } | |||
| } | |||
|
|
|||
| /// Returns a copy of this `LogicalPlan` with the new inputs | |||
| pub fn with_new_inputs(&self, inputs: &[LogicalPlan]) -> Result<LogicalPlan> { | |||
There was a problem hiding this comment.
I suggest we mark this deprecated for at least a release or two to help users migrate? I always apprecate the hint in the deprecation message that tells me what to change so I don't have to go poking around in the commit log to see what happened.
Perhaps it could call through to the with_new_exprs API, something like (untested):
#[deprecated(since = "35.0.0", note = "please use `with_new_exprs` instead")]
pub fn with_new_inputs(&self, inputs: &[LogicalPlan]) -> Result<LogicalPlan> {
self.with_new_exprs(self, self.expressions(), inputs)
}There was a problem hiding this comment.
Agreed. Marked it as deprecated one.
| assert_eq!(window_expr.len(), expr.len()); | ||
| Ok(LogicalPlan::Window(Window { | ||
| input: Arc::new(inputs[0].clone()), | ||
| window_expr: expr, | ||
| schema: schema.clone(), | ||
| })) |
There was a problem hiding this comment.
maybe @mustafasrepo remembers the context -- I vaguely remember something like this but can't remember the specifics
alamb
changed the title
with_new_inputsLogicalPlan::with_new_inputs
viirya
changed the title
LogicalPlan::with_new_inputsLogicalPlan::with_new_inputs
|
Thank you @alamb @mustafasrepo |
Which issue does this PR close?
Closes #.
Rationale for this change
This patch deprecates
with_new_inputsfunction which is duplicated withwith_new_exprs. We should not have duplicate APIs which cause confusion and maintenance issue.What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?