From b980e2a68524783fa8107877e3f2e2e08a32e1af Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Fri, 21 Jun 2024 19:38:40 +0800 Subject: [PATCH 1/3] fix: remove the Sized requirement on ExecutionPlan::name() Signed-off-by: Ruihang Xia --- datafusion/core/src/test/mod.rs | 4 ++++ datafusion/core/src/test_util/mod.rs | 4 ++++ .../core/tests/custom_sources_cases/mod.rs | 4 ++++ .../provider_filter_pushdown.rs | 4 ++++ .../tests/custom_sources_cases/statistics.rs | 4 ++++ .../tests/user_defined/user_defined_plan.rs | 4 ++++ datafusion/physical-plan/src/lib.rs | 15 +++++++----- datafusion/physical-plan/src/test/exec.rs | 24 +++++++++++++++++++ 8 files changed, 57 insertions(+), 6 deletions(-) diff --git a/datafusion/core/src/test/mod.rs b/datafusion/core/src/test/mod.rs index e91f83f1199b..2515b8a4e016 100644 --- a/datafusion/core/src/test/mod.rs +++ b/datafusion/core/src/test/mod.rs @@ -399,6 +399,10 @@ impl DisplayAs for StatisticsExec { } impl ExecutionPlan for StatisticsExec { + fn name(&self) -> &'static str { + Self::static_name() + } + fn as_any(&self) -> &dyn Any { self } diff --git a/datafusion/core/src/test_util/mod.rs b/datafusion/core/src/test_util/mod.rs index e876cfe46547..059fa8fc6da7 100644 --- a/datafusion/core/src/test_util/mod.rs +++ b/datafusion/core/src/test_util/mod.rs @@ -285,6 +285,10 @@ impl DisplayAs for UnboundedExec { } impl ExecutionPlan for UnboundedExec { + fn name(&self) -> &'static str { + Self::static_name() + } + fn as_any(&self) -> &dyn Any { self } diff --git a/datafusion/core/tests/custom_sources_cases/mod.rs b/datafusion/core/tests/custom_sources_cases/mod.rs index e8ead01d2ee4..e7e998c58211 100644 --- a/datafusion/core/tests/custom_sources_cases/mod.rs +++ b/datafusion/core/tests/custom_sources_cases/mod.rs @@ -140,6 +140,10 @@ impl DisplayAs for CustomExecutionPlan { } impl ExecutionPlan for CustomExecutionPlan { + fn name(&self) -> &'static str { + Self::static_name() + } + fn as_any(&self) -> &dyn Any { self } diff --git a/datafusion/core/tests/custom_sources_cases/provider_filter_pushdown.rs b/datafusion/core/tests/custom_sources_cases/provider_filter_pushdown.rs index 068383b20031..b5506b7c12f6 100644 --- a/datafusion/core/tests/custom_sources_cases/provider_filter_pushdown.rs +++ b/datafusion/core/tests/custom_sources_cases/provider_filter_pushdown.rs @@ -94,6 +94,10 @@ impl DisplayAs for CustomPlan { } impl ExecutionPlan for CustomPlan { + fn name(&self) -> &'static str { + Self::static_name() + } + fn as_any(&self) -> &dyn std::any::Any { self } diff --git a/datafusion/core/tests/custom_sources_cases/statistics.rs b/datafusion/core/tests/custom_sources_cases/statistics.rs index c7be89533f1d..2d42b03bfed8 100644 --- a/datafusion/core/tests/custom_sources_cases/statistics.rs +++ b/datafusion/core/tests/custom_sources_cases/statistics.rs @@ -145,6 +145,10 @@ impl DisplayAs for StatisticsValidation { } impl ExecutionPlan for StatisticsValidation { + fn name(&self) -> &'static str { + Self::static_name() + } + fn as_any(&self) -> &dyn Any { self } diff --git a/datafusion/core/tests/user_defined/user_defined_plan.rs b/datafusion/core/tests/user_defined/user_defined_plan.rs index 07622e48afaf..4c4268083732 100644 --- a/datafusion/core/tests/user_defined/user_defined_plan.rs +++ b/datafusion/core/tests/user_defined/user_defined_plan.rs @@ -459,6 +459,10 @@ impl DisplayAs for TopKExec { #[async_trait] impl ExecutionPlan for TopKExec { + fn name(&self) -> &'static str { + Self::static_name() + } + /// Return a reference to Any that can be used for downcasting fn as_any(&self) -> &dyn Any { self diff --git a/datafusion/physical-plan/src/lib.rs b/datafusion/physical-plan/src/lib.rs index bd77814bbbe4..d4620e815044 100644 --- a/datafusion/physical-plan/src/lib.rs +++ b/datafusion/physical-plan/src/lib.rs @@ -116,12 +116,7 @@ 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 - where - Self: Sized, - { - Self::static_name() - } + fn name(&self) -> &'static str; /// Short name for the ExecutionPlan, such as 'ParquetExec'. /// Like [`name`](ExecutionPlan::name) but can be called without an instance. @@ -829,6 +824,10 @@ mod tests { } impl ExecutionPlan for EmptyExec { + fn name(&self) -> &'static str { + Self::static_name() + } + fn as_any(&self) -> &dyn Any { self } @@ -881,6 +880,10 @@ mod tests { } impl ExecutionPlan for RenamedEmptyExec { + fn name(&self) -> &'static str { + Self::static_name() + } + fn static_name() -> &'static str where Self: Sized, diff --git a/datafusion/physical-plan/src/test/exec.rs b/datafusion/physical-plan/src/test/exec.rs index d5ad9292b49d..ad47a484c9f7 100644 --- a/datafusion/physical-plan/src/test/exec.rs +++ b/datafusion/physical-plan/src/test/exec.rs @@ -177,6 +177,10 @@ impl DisplayAs for MockExec { } impl ExecutionPlan for MockExec { + fn name(&self) -> &'static str { + Self::static_name() + } + fn as_any(&self) -> &dyn Any { self } @@ -335,6 +339,10 @@ impl DisplayAs for BarrierExec { } impl ExecutionPlan for BarrierExec { + fn name(&self) -> &'static str { + Self::static_name() + } + fn as_any(&self) -> &dyn Any { self } @@ -444,6 +452,10 @@ impl DisplayAs for ErrorExec { } impl ExecutionPlan for ErrorExec { + fn name(&self) -> &'static str { + Self::static_name() + } + fn as_any(&self) -> &dyn Any { self } @@ -527,6 +539,10 @@ impl DisplayAs for StatisticsExec { } impl ExecutionPlan for StatisticsExec { + fn name(&self) -> &'static str { + Self::static_name() + } + fn as_any(&self) -> &dyn Any { self } @@ -619,6 +635,10 @@ impl DisplayAs for BlockingExec { } impl ExecutionPlan for BlockingExec { + fn name(&self) -> &'static str { + Self::static_name() + } + fn as_any(&self) -> &dyn Any { self } @@ -760,6 +780,10 @@ impl DisplayAs for PanicExec { } impl ExecutionPlan for PanicExec { + fn name(&self) -> &'static str { + Self::static_name() + } + fn as_any(&self) -> &dyn Any { self } From 81aed71acac7cad5e103370c1c6ac1746a0db72f Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Sat, 22 Jun 2024 01:37:14 +0800 Subject: [PATCH 2/3] Update datafusion/physical-plan/src/lib.rs Co-authored-by: Andrew Lamb --- datafusion/physical-plan/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/physical-plan/src/lib.rs b/datafusion/physical-plan/src/lib.rs index d4620e815044..09a9b42f39bb 100644 --- a/datafusion/physical-plan/src/lib.rs +++ b/datafusion/physical-plan/src/lib.rs @@ -116,7 +116,7 @@ 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) -> &str; /// Short name for the ExecutionPlan, such as 'ParquetExec'. /// Like [`name`](ExecutionPlan::name) but can be called without an instance. From a3bc903ae72130ea0d18386b0dd616d9b76eaa60 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Sat, 22 Jun 2024 01:45:45 +0800 Subject: [PATCH 3/3] add document and test Signed-off-by: Ruihang Xia --- datafusion/physical-plan/src/lib.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/datafusion/physical-plan/src/lib.rs b/datafusion/physical-plan/src/lib.rs index 09a9b42f39bb..c648547c98b1 100644 --- a/datafusion/physical-plan/src/lib.rs +++ b/datafusion/physical-plan/src/lib.rs @@ -116,6 +116,12 @@ 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'. + /// + /// Implementation note: this method can just proxy to + /// [`static_name`](ExecutionPlan::static_name) if no special action is + /// needed. It doesn't provide a default implementation like that because + /// this method doesn't require the `Sized` constrain to allow a wilder + /// range of use cases. fn name(&self) -> &str; /// Short name for the ExecutionPlan, such as 'ParquetExec'. @@ -934,6 +940,14 @@ mod tests { assert_eq!(renamed_exec.name(), "MyRenamedEmptyExec"); assert_eq!(RenamedEmptyExec::static_name(), "MyRenamedEmptyExec"); } + + /// A compilation test to ensure that the `ExecutionPlan::name()` method can + /// be called from a trait object. + /// Related ticket: https://github.com/apache/datafusion/pull/11047 + #[allow(dead_code)] + fn use_execution_plan_as_trait_object(plan: &dyn ExecutionPlan) { + let _ = plan.name(); + } } pub mod test;