alamb commented on code in PR #10575:
URL: https://github.com/apache/datafusion/pull/10575#discussion_r1607048271
##########
datafusion/expr/src/logical_plan/extension.rs:
##########
@@ -76,27 +76,20 @@ pub trait UserDefinedLogicalNode: fmt::Debug + Send + Sync {
/// For example: `TopK: k=10`
fn fmt_for_explain(&self, f: &mut fmt::Formatter) -> fmt::Result;
- /// Create a new `ExtensionPlanNode` with the specified children
+ /// Create a new `UserDefinedLogicalNode` with the specified children
/// and expressions. This function is used during optimization
/// when the plan is being rewritten and a new instance of the
- /// `ExtensionPlanNode` must be created.
+ /// `UserDefinedLogicalNode` must be created.
///
/// Note that exprs and inputs are in the same order as the result
/// of self.inputs and self.exprs.
///
- /// So, `self.from_template(exprs, ..).expressions() == exprs
- //
- // TODO(clippy): This should probably be renamed to use a `with_*` prefix.
Something
- // like `with_template`, or `with_exprs_and_inputs`.
- //
- // Also, I think `ExtensionPlanNode` has been renamed to
`UserDefinedLogicalNode`
- // but the doc comments have not been updated.
- #[allow(clippy::wrong_self_convention)]
Review Comment:
Since this also changes the api signatures, would you be willing to leave
the old name in with a deprecated stub to ease migration for people (aka so the
compiler tells them what to do)
Somethinglike
```rust
#[deprecated(since = "39.0.0", note = "use with_exprs_and_inputs
instead")]
#[allow(clippy::wrong_self_convention)]
fn from_template(
&self,
exprs: &[Expr],
inputs: &[LogicalPlan],
) -> Arc<dyn UserDefinedLogicalNode> {
self.with_exprs_and_inputs(exprs, inputs).unwrap()
)
##########
datafusion/expr/src/logical_plan/extension.rs:
##########
@@ -76,27 +76,20 @@ pub trait UserDefinedLogicalNode: fmt::Debug + Send + Sync {
/// For example: `TopK: k=10`
fn fmt_for_explain(&self, f: &mut fmt::Formatter) -> fmt::Result;
- /// Create a new `ExtensionPlanNode` with the specified children
+ /// Create a new `UserDefinedLogicalNode` with the specified children
/// and expressions. This function is used during optimization
/// when the plan is being rewritten and a new instance of the
- /// `ExtensionPlanNode` must be created.
+ /// `UserDefinedLogicalNode` must be created.
///
/// Note that exprs and inputs are in the same order as the result
/// of self.inputs and self.exprs.
///
- /// So, `self.from_template(exprs, ..).expressions() == exprs
- //
- // TODO(clippy): This should probably be renamed to use a `with_*` prefix.
Something
- // like `with_template`, or `with_exprs_and_inputs`.
- //
- // Also, I think `ExtensionPlanNode` has been renamed to
`UserDefinedLogicalNode`
- // but the doc comments have not been updated.
- #[allow(clippy::wrong_self_convention)]
- fn from_template(
+ /// So, `self.with_exprs_and_inputs(exprs, ..).expressions() == exprs
+ fn with_exprs_and_inputs(
&self,
exprs: &[Expr],
inputs: &[LogicalPlan],
- ) -> Arc<dyn UserDefinedLogicalNode>;
+ ) -> Result<Arc<dyn UserDefinedLogicalNode>>;
Review Comment:
If we are going to change the signature anyways, what would you think about
changing it to an API that doesn't require cloning the arguments -- for example
```rust
fn with_exprs_and_inputs(
&self,
exprs: Vec<Expr>,
inputs: Vec<LogicalPlan>,
) -> Result<Arc<dyn UserDefinedLogicalNode>>;
```
--
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
