Add name method to ExecutionPlan trait

[matthewmturner]

Is your feature request related to a problem or challenge?

In the telemetry we produce on query executions we sometimes need to know the name of ExecutionPlan nodes (i.e. ParquetExec). Right now we do something like the following to extract the name:

// Get the one-line representation of the ExecutionPlan, something like this:
//   ParquetExec: file_groups=[...], ...
let mut buf = String::new();
write!(&mut buf, "{}", displayable(plan).one_line()).map_err(|e| {
    DataFusionError::Internal(format!("Error while collecting metrics: {e}"))
 })?;
 let node_type = match buf.split_once(':') {
    None => {
        warn!("execution plan has unexpected display format: {buf}");
           return Ok(true);
    }
   Some((name, _)) => name.to_string(),
 };

From what I have seen the name of ExecutionPlan is always known at compile time so I think this is unnecessary work and I am hoping to provide a simpler way to get access to this information.

Describe the solution you'd like

Could we add something like the following to ExecutionPlan

pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync {
    const NAME: &'static str;

    fn name(&self) -> &'static str {
        return Self::NAME;
    }

impl ExecutionPlan for ParquetExec {
    const NAME = "ParquetExec";

    // Rest of `ParquetExec` is unchanged
    ....
}

Describe alternatives you've considered

No response

Additional context

No response

[SteveLauC]

We can probably impl this method using td::any::type_name and make it a provided method:

Something like

fn name(&self) -> &str {
    let full_name = std::any::type_name::<Self>();
    let start_idx = full_name.rfind(':').unwrap() + 1;

    &full_name[start_idx..]
}

}

[matthewmturner]

I was hoping to avoid having to do any compute since I believe the name is always known at compile time. Of course my proposal would make this a breaking change and maybe thats not wanted for this type of change. If so, then certainly we could do something as a provided method.

[matthewmturner]

@alamb im curious if you have thoughts on this.

[alamb]

I think adding a name() function to ExecutionPlan sounds like a good idea to me

Requiring a &static str would likely be inconvenient for some use cases, but given the API doesn't currently exist I think it would be fine if you wanted to add it.

Note I think you could probably just add a single function and make the change to the trait backwards compatible, for example

pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync {
    /// return the name of this execution plan's type
    fn name(&self) -> &'static str { "UNKNOWN" }
...
}

The UserDefinedLogicalNode has a fn name(&self) -> &str: https://docs.rs/datafusion/latest/datafusion/logical_expr/trait.UserDefinedLogicalNode.html#tymethod.name
Which is another possiblity

Another option for an API that avoids string copying could be

fn name(&self) -> Arc<str> ;

To still allow dynamicc names

[matthewmturner]

Note I think you could probably just add a single function and make the change to the trait backwards compatible, for example

pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync {
    /// return the name of this execution plan's type
    fn name(&self) -> &'static str { "UNKNOWN" }
...
}

I think thats a great idea.

Another option for an API that avoids string copying could be

fn name(&self) -> Arc<str> ;

To still allow dynamicc names

Do you have or know of existing use cases for dynamic names? I had in mind that these names would always be known at compile time.

[alamb]

Do you have or know of existing use cases for dynamic names? I had in mind that these names would always be known at compile time.

I do not know of any use cases for dynamic names