Add name method to execution plan #9793
Conversation
| @@ -113,6 +113,10 @@ pub use datafusion_execution::{RecordBatchStream, SendableRecordBatchStream}; | |||
| /// [`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'. | |||
| fn name(&self) -> &'static str { | |||
There was a problem hiding this comment.
should we have a default impl here? the user might forget to override the name
After rethinking I think that that option is indeed a better default. I will update to that. FYI @SteveLauC |
|
@comphead @SteveLauC I have updated the default impl. Let me know if any more concerns. |
| @@ -113,6 +113,15 @@ pub use datafusion_execution::{RecordBatchStream, SendableRecordBatchStream}; | |||
| /// [`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'. | |||
| fn name(&self) -> &'static str { | |||
| let full_name = std::any::type_name::<Self>(); | |||
There was a problem hiding this comment.
Just realized that users can implement their own implementations if they don't want this method to involve any computation, nice!
There was a problem hiding this comment.
thanks @matthewmturner I think we really close.
Reg to test, wondering can it be smaller? I'd say we dont need the full implementations for trait methods, just stubs because you testing out just .name() field and its overrides?
Instead of implementations you can put macros like todo()!, 'unreacheable()!', 'not_implemented()!'
|
@comphead sure, i didnt realize those could override the type analysis. updating now |
There was a problem hiding this comment.
Thanks @matthewmturner lgtm
1 more thing is probably we need to ensure the name is unique? if implementation will be separated across different crates and users can develop their own implementations it can possibly be that the name will not be unique for 2 different structs
|
@comphead thats a good / interesting point. what comes to mind is perhaps datafusion statically defining / guaranteeing uniqueness of builtin exec names (perhaps could be automated?) and then users would be required to ensure uniqueness of their own execs. im not sure we would want to add some runtime check for that. i think this could be a follow on PR but let me know if you think otherwise. |
|
I'm okay with the followup, as there is only 1 use case for now, but we need to keep this in mind |
|
Thanks everyone! |
* Add name method to execution plan * Cleanup * Change default impl * Add tests * Clippy * Use unimplemented macro * Fix
Which issue does this PR close?
Closes #9558
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?