fix: remove the Sized requirement on ExecutionPlan::name() #11047
Conversation
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
|
Could we add a test to show that |
| /// [`required_input_distribution`]: ExecutionPlan::required_input_distribution | ||
| /// [`required_input_ordering`]: ExecutionPlan::required_input_ordering | ||
| pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync { | ||
| /// Short name for the ExecutionPlan, such as 'ParquetExec'. |
There was a problem hiding this comment.
Perhaps we can also update the doc string to include a note about using static_name if desired to help people migrate
There was a problem hiding this comment.
Great point 👍 I add an implementation note section in doc string
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Certainly 👍 Add a compilation test |
There was a problem hiding this comment.
Looks good to me -- thank you @waynexia and @matthewmturner
| assert_eq!(RenamedEmptyExec::static_name(), "MyRenamedEmptyExec"); | ||
| } | ||
|
|
||
| /// A compilation test to ensure that the `ExecutionPlan::name()` method can |
There was a problem hiding this comment.
Looks great to me -- thank you @waynexia and @matthewmturner
|
Thanks @matthewmturner @alamb ❤️ |
) * fix: remove the Sized requirement on ExecutionPlan::name() Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * Update datafusion/physical-plan/src/lib.rs Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> * add document and test Signed-off-by: Ruihang Xia <waynestxia@gmail.com> --------- Signed-off-by: Ruihang Xia <waynestxia@gmail.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
) * fix: remove the Sized requirement on ExecutionPlan::name() Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * Update datafusion/physical-plan/src/lib.rs Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> * add document and test Signed-off-by: Ruihang Xia <waynestxia@gmail.com> --------- Signed-off-by: Ruihang Xia <waynestxia@gmail.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> (cherry picked from commit a19fc62)
3 participants
Which issue does this PR close?
Follow-up of #10266
Rationale for this change
#10266 tries to provide a default implementation based on
static_name(). This propagates theSizedrequirement toname()and prohibits it from being called by a trait object.What changes are included in this PR?
Remove the default implementation of
ExecutionPlan::name()and let every implementor implement their own based onstatic_name()to workaround object safety requirements. This is something likeas_any().This solution is not elegant, but I can only find this way to suit both (1) invoke from trait object and (2) invoke from type name. Please add your suggestion if there is any better way to do so. Thanks in advance!
Are these changes tested?
Are there any user-facing changes?
Yes, for those who implements
ExecutionPlan, they need to add a new method onimpl