-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve UserDefinedLogicalNode::from_template
API to return Result
#10575
Improve UserDefinedLogicalNode::from_template
API to return Result
#10575
Conversation
UserDefinedLogicalNode::from_template
APIUserDefinedLogicalNode::from_template
API to return Result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @lewiszlw -- this is great.
I had a few suggestions on the API design, let me know what you think
If we want to apply same change to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to apply same change to UserDefinedLogicalNodeCore trait, we shoud add Sized trait bound to it, do you need me to change it in this pr? @alamb
I think it would be ok to do in a follow on PR. My rationale being that this code is usable in its current form, though UserDefinedLogicalNodeCore is often used too
Are you willing to do so, that would be great. Otherwise let's file a ticket to track.
Thanks again @lewiszlw
@@ -76,27 +76,31 @@ 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 | |||
#[deprecated(since = "39.0.0", note = "use with_exprs_and_inputs instead")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
apache#10575) * UserDefinedLogicalNode::from_template return Result * Rename from_template to with_exprs_and_inputs * Resolve review comments
Which issue does this PR close?
Closes #10571.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?