feat: add name() method to UserDefinedLogicalNode #5450
Conversation
Signed-off-by: Ruihang Xia <[email protected]>
github-actions
bot
added
core
|
@waynexia wouldn't this better to use enum for user node names? |
I'm afraid it's hard to define this enum because we can't enumerate all the possible node in advance 🧐 |
There was a problem hiding this comment.
Thank you @waynexia -- I think it may be useful to see if we can combine this somehow with format_for_explain (as mentioned in my comments) but this also seems like useful information as well so I would be fine with merging it as is.
| @@ -30,6 +30,9 @@ pub trait UserDefinedLogicalNode: fmt::Debug + Send + Sync { | |||
| /// Return a reference to self as Any, to support dynamic downcasting | |||
| fn as_any(&self) -> &dyn Any; | |||
|
|
|||
| /// Return the plan's name | |||
There was a problem hiding this comment.
I think similar information is present via
fn fmt_for_explain(&self, f: &mut fmt::Formatter) -> fmt::Result;
However, it is not easy to use (as you need a formatter, etc)
What would you think about making a function like the following that returned the result of format_for_explain
fn to_string(&self) -> String {
// figure out how to call format_for_explain here
}There was a problem hiding this comment.
I ignored the fmt_for_explain method... You are right, they are redundant information.
What would you think about making a function like the following that returned the result of format_for_explain
The returned string (like "TopK: k=1") is also not easy to use for the dispatching purpose, like we need to .contains() or .start_with() those strings. What do you think about separate name and parameters? E.g.:
fn name(&self) -> &'str {
"TopK"
}
fn fmt_for_params(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "k={}", self.k)
}
// this can be provided as default implementation
fn fmt_for_explain(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "{}: {}", self.name(), self.fmt_for_params())
}There was a problem hiding this comment.
What do you think about separate name and parameters? E.g.:
I think it would make sense, but could also be done as a follow on PR. I think this PR is good enough for now.
Thanks @waynexia
|
Benchmark runs are scheduled for baseline = eb95413 and contender = c56b947. c56b947 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #5449.
Rationale for this change
Add
name()method so that there is no need to test aUserDefinedLogicalNodeusingdowncast_ref().What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?
The public interface
UserDefinedLogicalNodeis changed