Skip to content
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

feat: add static_name() to ExecutionPlan #10266

Merged
merged 2 commits into from
Apr 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions datafusion/physical-plan/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,27 @@ pub mod udaf {
/// [`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 {
fn name(&self) -> &'static str
where
Self: Sized,
{
Self::static_name()
}
Comment on lines +120 to +125
Copy link
Contributor

@joroKr21 joroKr21 Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we can't call plan.name() inside ExecutionPlanVisitor:

    fn pre_visit(&mut self, plan: &dyn ExecutionPlan) -> Result<bool, Self::Error> {
        let plan_name = plan.name().to_owned();
        ...
    }
error: the `name` method cannot be invoked on a trait object
     |
1552 |         let plan_name = plan.name().to_owned();
     |                              ^^^^
     |
     |
121  |         Self: Sized,
     |               ----- this has a `Sized` requirement

Copy link
Member Author

@waynexia waynexia Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this method cannot be called from a trait object where Sized is not required.

I'm considering making a breaking API change to deprecate the default implementation and provide the static name on each impl. This can avoid the issue we're facing now and also keep other functionalities working like before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's nice to be able to call name() on the dyn object.
As long as we can keep doing that I would be happy 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I'll submit a patch later this day (or tomorrow).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI #11047


/// Short name for the ExecutionPlan, such as 'ParquetExec'.
/// Like [`name`](ExecutionPlan::name) but can be called without an instance.
fn static_name() -> &'static str
where
Self: Sized,
{
let full_name = std::any::type_name::<Self>();
let maybe_start_idx = full_name.rfind(':');
match maybe_start_idx {
Some(start_idx) => &full_name[start_idx + 1..],
None => "UNKNOWN",
}
}

/// Returns the execution plan as [`Any`] so that it can be
/// downcast to a specific implementation.
fn as_any(&self) -> &dyn Any;
Expand Down Expand Up @@ -873,7 +886,10 @@ mod tests {
}

impl ExecutionPlan for RenamedEmptyExec {
fn name(&self) -> &'static str {
fn static_name() -> &'static str
where
Self: Sized,
{
"MyRenamedEmptyExec"
}

Expand Down Expand Up @@ -918,6 +934,7 @@ mod tests {
let schema2 = Arc::new(Schema::empty());
let renamed_exec = RenamedEmptyExec::new(schema2);
assert_eq!(renamed_exec.name(), "MyRenamedEmptyExec");
assert_eq!(RenamedEmptyExec::static_name(), "MyRenamedEmptyExec");
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ type SharedGate = Arc<Gate>;

#[cfg(test)]
mod tests {
use std::sync::atomic::{AtomicBool, Ordering};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR but it fails clippy check in my env.

use std::sync::atomic::AtomicBool;

use futures::{task::ArcWake, FutureExt};

Expand Down