geoffreyclaude opened a new pull request, #23154: URL: https://github.com/apache/datafusion/pull/23154
## Which issue does this PR close? - Does not close an issue. Follow-up to #22559 and #21263. ## Rationale for this change #21263 removed `ExecutionPlan::as_any()` now that `ExecutionPlan` has `Any` as a supertrait. Most call sites were migrated from code like this: ```rust plan.as_any().downcast_ref::<SomeExec>() ``` to the new helper: ```rust plan.downcast_ref::<SomeExec>() ``` That distinction matters after #22559. `ExecutionPlan::downcast_ref` is now the public downcast operation for execution plans: it follows `ExecutionPlan::downcast_delegate()` before falling back to raw `Any`. This lets wrapper plans keep their own concrete type private while still presenting the wrapped plan's normal public identity. For example, tracing or instrumentation wrappers can implement `downcast_delegate()` so callers that ask "is this a `FilterExec` / `EmptyExec` / `ProjectionExec`?" get the same answer they would have received without the wrapper. The wrapper remains visible only to code that intentionally performs a raw concrete-type `Any` check. The physical plan proto serializer still had one leftover mechanical migration from the old `as_any()` world: ```rust let plan = plan.as_ref() as &dyn Any; ``` That line bypasses `ExecutionPlan::downcast_ref` entirely. As a result, a delegating wrapper around a built-in execution plan is not serialized as the built-in plan. Instead, the serializer sees only the wrapper's concrete type, fails all built-in `Exec` checks, and falls through to the extension codec. With the default extension codec this produces an unsupported-plan error, even though the wrapped plan itself is serializable. This is particularly relevant for transparent wrappers such as `InstrumentedExec` in `datafusion-contrib/datafusion-tracing`, which intentionally delegate public downcasts to the wrapped plan. ## What changes are included in this PR? - Changes physical plan proto serialization to bind the plan as `&dyn ExecutionPlan` instead of `&dyn Any`. - Leaves the existing serializer downcast chain intact, so each `plan.downcast_ref::<...>()` now dispatches through the `ExecutionPlan` helper and therefore honors `downcast_delegate()`. - Adds a regression test with a small wrapper execution plan that delegates public downcasts to an inner `EmptyExec`. - Audited other `as_ref() as &dyn Any` patterns. The remaining hits are either non-`ExecutionPlan` traits, `PhysicalExpr` checks, UDF tests, or the intentional raw `Any` fallback inside the `ExecutionPlan` helper itself. ## Are these changes tested? Yes. ```bash cargo fmt --all cargo fmt --all --check cargo test -p datafusion-proto --test proto_integration serialize_uses_downcast_delegate cargo clippy --all-targets --all-features -- -D warnings ``` ## Are there any user-facing changes? Yes, as a bug fix. Physical plan proto serialization now honors the documented `ExecutionPlan::downcast_delegate()` behavior. Transparent wrapper plans can serialize as their delegated built-in execution plan when appropriate instead of requiring an extension codec for the wrapper itself. -- 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]
