chore: remove as_any from ExecutionPlan by timsaucer · Pull Request #21263 · apache/datafusion

timsaucer · 2026-03-30T20:29:07Z

Which issue does this PR close?

This is a follow on to #20812 and #21209 but treats ExecutionPlan.

Rationale for this change

This PR reduces the amount of boilerplate code that users need to write for execution plans.

What changes are included in this PR?

Now that we have trait upcasting since rust 1.86, we no longer need every implementation of these functions to have the as_any function that returns &self. This PR makes Any an supertrait and makes the appropriate casts when necessary.

I have also implemented functions is and downcast_ref on the trait object for ExecutionPlan and applied this same pattern to the udf, udaf, and udwf implementations. This allows for a clean downcasting and type checking.

Are these changes tested?

Existing unit tests.

Are there any user-facing changes?

Yes, the users simply need to remove the as_any function. The upgrade guide is updated.

When upcasting to `&dyn Any` from an `Arc<dyn Trait>`, using `(&plan as &dyn Any)` gives an Any for the Arc, not the inner trait object. Document that `.as_ref() as &dyn Any` is required instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

alamb

Thanks @timsaucer

The basic idea here is great -- however, this pattern of calling .as_any().downcast_ref()... is super common over the codebase and changing it to (scan.as_ref() as &dyn Any) is pretty 🤮

So in other words, I am not sure this PR is a good as it makes the code harder to read (at least for me). Is there anyway to avoid this (like maybe keeping the as_any as a default impl perhaps?)

alamb · 2026-03-31T20:10:11Z


        fn f_down(&mut self, node: &'n Self::Node) -> Result<TreeNodeRecursion> {
-            if let Some(exec) = node.as_any().downcast_ref::<AggregateExec>() {
+            if let Some(exec) =


I will be honest that the previous syntax is quite a bit nicer 🤔

You're right. Moved to draft, at least for the time being.

Thank you for the push back @alamb. If you take another look, I think we now have a very clean downcast. I also updated the UDFs at the same time and applied the pattern. They have many fewer places where we do downcast_ref or is so it wasn't so noticeable.

Adds an inherent impl on `dyn ExecutionPlan` providing `downcast_ref<T>` and `is<T>` methods, so callers can write `plan.downcast_ref::<MyExec>()` instead of `(plan.as_ref() as &dyn Any).downcast_ref::<MyExec>()`. Both methods work correctly whether `plan` is `&dyn ExecutionPlan` or `Arc<dyn ExecutionPlan>` via auto-deref. Updates call sites in tests and the 54.0.0 upgrade guide accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…l, WindowUDFImpl Mirrors the same inherent impl pattern added for dyn ExecutionPlan. Callers can now write `udf.downcast_ref::<MyUdf>()` instead of `(udf.as_ref() as &dyn Any).downcast_ref::<MyUdf>()`, and it works correctly whether the value is a bare reference or behind an Arc. Updates the 54.0.0 upgrade guide accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Replaces all `(plan.as_ref() as &dyn Any).downcast_ref::<T>()` and `(plan as &dyn Any).is::<T>()` call sites with the new inherent methods `plan.downcast_ref::<T>()` and `plan.is::<T>()` added to `dyn ExecutionPlan`. Also removes the now-unused `use std::any::Any` imports. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Replaces remaining `(x.as_ref() as &dyn Any).downcast_ref::<T>()` and `(x as &dyn std::any::Any).downcast_ref::<T>()` patterns with the new inherent methods on `dyn ExecutionPlan`. Also cleans up redundant `.as_ref()` calls before `.downcast_ref()` / `.is()` on Arc values, and simplifies `.downcast_ref::<T>().is_some()` to `.is::<T>()` where applicable. Removes now-unused `std::any::Any` imports. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Fixes two missed ExecutionPlan patterns: - topk_repartition.rs: remove intermediate as &dyn Any variable - limit_pushdown.rs: remove intermediate as &dyn Any variable Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Replaces `(func.inner().as_ref() as &dyn Any).downcast_ref::<T>()` and `.is::<T>()` call sites with the new inherent methods on `dyn ScalarUDFImpl`. Also simplifies `.downcast_ref::<T>().is_some()` to `.is::<T>()` and removes now-unused `std::any::Any` imports. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…tterns Replaces `(udaf.inner().as_ref() as &dyn Any).downcast_ref::<T>()` and `.is::<T>()` call sites with the new inherent methods on `dyn AggregateUDFImpl`. Removes now-unused `std::any::Any` imports. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Replaces `(udwf.inner().as_ref() as &dyn Any).downcast_ref::<T>()` and `.is::<T>()` call sites with the new inherent methods on `dyn WindowUDFImpl`. Removes now-unused `std::any::Any` imports. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

timsaucer · 2026-04-01T13:01:22Z

    pub fn try_downcast_func<T>(expr: &dyn PhysicalExpr) -> Option<&ScalarFunctionExpr>
    where
-        T: 'static,
+        T: ScalarUDFImpl,


This may look like it's changing behavior because we're changing the generic but if you look at the usage, the downcast must be to a ScalarUDFImpl so this is valid IMO.

…pplied it to expressions

alamb

This is much nicer

alamb · 2026-04-02T17:44:08Z

 }

+impl dyn ScalarUDFImpl {
+    /// Returns `true` if the implementation is of type `T`.


this is nice

alamb · 2026-04-02T17:44:43Z

+ }
+```
+
+If you have code that is downcasting, you can use the new `downcast_ref`


alamb · 2026-04-02T17:45:05Z

+whether the value is a bare reference or behind an `Arc` (Rust
+auto-derefs through the `Arc`).
+
+> **Warning:** Do not cast an `Arc<dyn Trait>` directly to `&dyn Any`.


is this meant to have > in front of it?

Yes, so it renders like this:

…d CatalogProviderList (#21346) ## Which issue does this PR close? This is a follow on to #20812 #21209 #21263 but treats `TableProvider`, `SchemaProvider`, `CatalogProvider`, and `CatalogProviderList`. ## Rationale for this change This PR reduces the amount of boilerplate code that users need to write for catalogs through table providers. ## What changes are included in this PR? Now that we have [trait upcasting](https://blog.rust-lang.org/2025/04/03/Rust-1.86.0/) since rust 1.86, we no longer need every implementation of these functions to have the as_any function that returns &self. This PR makes Any an supertrait and makes the appropriate casts when necessary. ## Are these changes tested? Existing unit tests. ## Are there any user-facing changes? Yes, the users simply need to remove the `as_any` function. The upgrade guide is updated. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…d CatalogProviderList (apache#21346) ## Which issue does this PR close? This is a follow on to apache#20812 apache#21209 apache#21263 but treats `TableProvider`, `SchemaProvider`, `CatalogProvider`, and `CatalogProviderList`. ## Rationale for this change This PR reduces the amount of boilerplate code that users need to write for catalogs through table providers. ## What changes are included in this PR? Now that we have [trait upcasting](https://blog.rust-lang.org/2025/04/03/Rust-1.86.0/) since rust 1.86, we no longer need every implementation of these functions to have the as_any function that returns &self. This PR makes Any an supertrait and makes the appropriate casts when necessary. ## Are these changes tested? Existing unit tests. ## Are there any user-facing changes? Yes, the users simply need to remove the `as_any` function. The upgrade guide is updated. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

## Which issue does this PR close? This is a follow on to apache#20812 and apache#21209 but treats `ExecutionPlan`. ## Rationale for this change This PR reduces the amount of boilerplate code that users need to write for execution plans. ## What changes are included in this PR? Now that we have [trait upcasting](https://blog.rust-lang.org/2025/04/03/Rust-1.86.0/) since rust 1.86, we no longer need every implementation of these functions to have the as_any function that returns &self. This PR makes Any an supertrait and makes the appropriate casts when necessary. I have also implemented functions `is` and `downcast_ref` on the trait object for ExecutionPlan and applied this same pattern to the udf, udaf, and udwf implementations. This allows for a clean downcasting and type checking. ## Are these changes tested? Existing unit tests. ## Are there any user-facing changes? Yes, the users simply need to remove the `as_any` function. The upgrade guide is updated. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…d CatalogProviderList (apache#21346) ## Which issue does this PR close? This is a follow on to apache#20812 apache#21209 apache#21263 but treats `TableProvider`, `SchemaProvider`, `CatalogProvider`, and `CatalogProviderList`. ## Rationale for this change This PR reduces the amount of boilerplate code that users need to write for catalogs through table providers. ## What changes are included in this PR? Now that we have [trait upcasting](https://blog.rust-lang.org/2025/04/03/Rust-1.86.0/) since rust 1.86, we no longer need every implementation of these functions to have the as_any function that returns &self. This PR makes Any an supertrait and makes the appropriate casts when necessary. ## Are these changes tested? Existing unit tests. ## Are there any user-facing changes? Yes, the users simply need to remove the `as_any` function. The upgrade guide is updated. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

apache#23154) ## Which issue does this PR close? N/A. This is a follow-up to apache#22559 and apache#21263, but there is no dedicated issue for this specific bug. ## Rationale for this change apache#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 apache#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. This problem came up while trying to instrument distributed plans through [`datafusion-distributed`](https://github.com/datafusion-contrib/datafusion-distributed). In that setting, physical plans may be wrapped for tracing/instrumentation and then serialized for distributed execution. The wrapper is meant to be transparent for normal plan inspection, but serialization still needs to recognize the delegated built-in plan underneath it. 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`](https://github.com/datafusion-contrib/datafusion-tracing/blob/main/datafusion-tracing/src/instrumented_exec.rs) in [`datafusion-contrib/datafusion-tracing`](https://github.com/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.

Remove as_any function from execution plans and use trait upcasting

13ae590

timsaucer and others added 6 commits March 30, 2026 16:29

Merge branch 'main' into chore/remove-as-any-execution-plan

d1bb4ad

we already had &dyn so no need for as_ref()

c6e9e53

Update unit tests

5562887

Use Any::is() instead of downcast_ref().is_some() in tests

7a4b653

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Merge branch 'main' into chore/remove-as-any-execution-plan

5d3a083

timsaucer marked this pull request as ready for review March 31, 2026 17:47

alamb reviewed Mar 31, 2026

View reviewed changes

timsaucer marked this pull request as draft March 31, 2026 20:19

timsaucer and others added 3 commits March 31, 2026 16:40

github-actions Bot added the logical-expr Logical plan and expressions label Mar 31, 2026

timsaucer and others added 5 commits April 1, 2026 07:14

Clean up remaining ExecutionPlan as &dyn Any patterns

c36b282

Fixes two missed ExecutionPlan patterns: - topk_repartition.rs: remove intermediate as &dyn Any variable - limit_pushdown.rs: remove intermediate as &dyn Any variable Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

github-actions Bot added physical-expr Changes to the physical-expr crates functions Changes to functions implementation labels Apr 1, 2026

timsaucer commented Apr 1, 2026

View reviewed changes

We got a little too carried away with the 'as &dyn Any' pattern and a…

9233df3

…pplied it to expressions

timsaucer marked this pull request as ready for review April 1, 2026 13:16

alamb approved these changes Apr 2, 2026

View reviewed changes

alamb added the api change Changes the API exposed to users of the crate label Apr 2, 2026

timsaucer enabled auto-merge April 3, 2026 12:00

timsaucer disabled auto-merge April 3, 2026 12:00

Merge branch 'main' into chore/remove-as-any-execution-plan

1109532

timsaucer added this pull request to the merge queue Apr 3, 2026

Merged via the queue into apache:main with commit dd8760d Apr 3, 2026
35 checks passed

timsaucer deleted the chore/remove-as-any-execution-plan branch April 3, 2026 13:33

timsaucer mentioned this pull request Apr 3, 2026

remove as_any from TableProvider, SchemaProvider, CatalogProvider, and CatalogProviderList #21346

Merged

This was referenced May 27, 2026

ExecutionPlan downcasting no longer supports transparent wrapper nodes #22557

Closed

Release DataFusion 54.0.0 (Apr 2026 / May 2026) #21080

Closed

xanderbailey mentioned this pull request Jun 14, 2026

chore(deps): Bump DataFusion to 54.0.0 apache/iceberg-rust#2648

Open

This was referenced Jun 24, 2026

fix(proto): honor ExecutionPlan downcast_delegate during serialization #23154

Merged

fix(proto): honor ExecutionPlan downcast_delegate during serialization DataDog/datafusion#149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore: remove as_any from ExecutionPlan#21263

chore: remove as_any from ExecutionPlan#21263
timsaucer merged 17 commits into
apache:mainfrom
timsaucer:chore/remove-as-any-execution-plan

timsaucer commented Mar 30, 2026 •

edited

Loading

Uh oh!

alamb left a comment

Uh oh!

alamb Mar 31, 2026

Uh oh!

timsaucer Mar 31, 2026

Uh oh!

timsaucer Apr 1, 2026

Uh oh!

timsaucer Apr 1, 2026

Uh oh!

alamb left a comment

Uh oh!

alamb Apr 2, 2026

Uh oh!

alamb Apr 2, 2026

Uh oh!

alamb Apr 2, 2026

Uh oh!

timsaucer Apr 2, 2026

Uh oh!

alamb Apr 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

timsaucer commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

timsaucer commented Mar 30, 2026 •

edited

Loading