geoffreyclaude opened a new issue, #22557: URL: https://github.com/apache/datafusion/issues/22557
### Describe the bug DataFusion 54 changed `ExecutionPlan` downcasting in PR #21263 by removing `ExecutionPlan::as_any`, making `ExecutionPlan` extend `Any`, and adding inherent `dyn ExecutionPlan::is::<T>()` / `downcast_ref::<T>()` helpers. That removes boilerplate for normal execution plan nodes, but it also removes a behavior that transparent wrapper nodes could previously implement: exposing the wrapped inner node for downcasting while keeping the wrapper as an implementation detail. A public example is [`datafusion-tracing`](https://github.com/datafusion-contrib/datafusion-tracing). It wraps `ExecutionPlan` nodes to record spans, metrics, and optional previews. Before DataFusion 54, its wrapper could keep itself mostly invisible by implementing `as_any()` to return the inner plan’s `as_any()` for normal callers, while using crate-internal logic to identify already-wrapped nodes during its own optimizer pass. With the new implementation: ```rust impl dyn ExecutionPlan { pub fn is<T: ExecutionPlan>(&self) -> bool { (self as &dyn Any).is::<T>() } pub fn downcast_ref<T: ExecutionPlan>(&self) -> Option<&T> { (self as &dyn Any).downcast_ref() } } ``` a wrapper can no longer influence downcasting. `is::<T>()` and `downcast_ref::<T>()` always see the concrete wrapper type, not the wrapped plan type. ### To Reproduce Conceptually: ```rust #[derive(Debug)] struct TransparentWrapper { inner: Arc<dyn ExecutionPlan>, } impl ExecutionPlan for TransparentWrapper { // delegates most methods to `inner` // This was possible before DataFusion 54: // fn as_any(&self) -> &dyn Any { // self.inner.as_any() // } } let inner: Arc<dyn ExecutionPlan> = Arc::new(SomeExec::new(...)); let wrapped: Arc<dyn ExecutionPlan> = Arc::new(TransparentWrapper { inner }); assert!(wrapped.downcast_ref::<SomeExec>().is_some()); // worked with delegated `as_any` assert!(wrapped.downcast_ref::<SomeExec>().is_none()); // current DataFusion 54 behavior ``` The exact wrapper may be for tracing, metrics, streaming instrumentation, caching, distribution, or other cross-cutting behavior. The common pattern is that the wrapper is intended to preserve the wrapped plan’s public identity. ### Expected behavior There should be a supported way for wrapper `ExecutionPlan` implementations to opt into transparent type introspection, or the new behavior should be documented as an intentional breaking semantic change for wrappers. One possible API shape would be an explicit hook such as: ```rust trait ExecutionPlan: Any + Debug + DisplayAs + Send + Sync { fn transparent_child(&self) -> Option<&dyn ExecutionPlan> { None } } ``` Then `dyn ExecutionPlan::is::<T>()` / `downcast_ref::<T>()` could check the concrete object first and, if needed, follow the transparent child. The exact API is not important; the useful property is an upstream contract for wrappers that intentionally preserve the wrapped node’s public identity. ### Additional context This came up while preparing `datafusion-tracing` for DataFusion 54. The crate can keep its wrapper type private in Rust API terms, but it cannot preserve the old transparent downcasting behavior without upstream support because the new `dyn ExecutionPlan` methods are inherent methods, not trait methods that a wrapper can override. This affects code that traverses physical plans and detects specific node types via `is::<T>()` or `downcast_ref::<T>()`, including optimizers, serializers/codecs, estimators, and observability tooling. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
