Skip to content

chore: remove as_any from ExecutionPlan#21263

Open
timsaucer wants to merge 16 commits intoapache:mainfrom
timsaucer:chore/remove-as-any-execution-plan
Open

chore: remove as_any from ExecutionPlan#21263
timsaucer wants to merge 16 commits intoapache:mainfrom
timsaucer:chore/remove-as-any-execution-plan

Conversation

@timsaucer
Copy link
Copy Markdown
Member

@timsaucer timsaucer commented Mar 30, 2026 •
edited
Loading

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.

@github-actions github-actions bot added documentation Improvements or additions to documentation optimizer Optimizer rules core Core DataFusion crate substrait Changes to the substrait crate catalog Related to the catalog crate proto Related to proto crate datasource Changes to the datasource crate ffi Changes to the ffi crate physical-plan Changes to the physical-plan crate labels Mar 30, 2026
timsaucer and others added 6 commits March 30, 2026 16:29
@timsaucer
Merge branch 'main' into chore/remove-as-any-execution-plan
d1bb4ad
@timsaucer
we already had &dyn so no need for as_ref()
c6e9e53
@timsaucer
Update unit tests
5562887
@timsaucer @claude
Add warning about Arc<dyn Trait> downcasting pitfall in upgrade guide
cde29a5 
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>
@timsaucer @claude
Use Any::is() instead of downcast_ref().is_some() in tests
7a4b653 
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@timsaucer
Merge branch 'main' into chore/remove-as-any-execution-plan
5d3a083
@timsaucer timsaucer marked this pull request as ready for review March 31, 2026 17:47
alamb
alamb reviewed Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

datafusion/core/tests/fuzz_cases/aggregate_fuzz.rs Outdated

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) =
Copy link
Copy Markdown
Contributor

@alamb alamb Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@timsaucer timsaucer Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@timsaucer timsaucer Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@timsaucer timsaucer marked this pull request as draft March 31, 2026 20:19
timsaucer and others added 3 commits March 31, 2026 16:40
@timsaucer @claude
Add downcast_ref and is methods to dyn ExecutionPlan
da69a26 
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>
@timsaucer @claude
Add downcast_ref and is methods to dyn ScalarUDFImpl, AggregateUDFImp…
ec73f02 
…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>
@timsaucer @claude
Use ExecutionPlan::downcast_ref and is() instead of as &dyn Any patterns
0a2881f 
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>
@github-actions 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
@timsaucer @claude
Use ExecutionPlan::downcast_ref and is() instead of as &dyn Any patterns
5c8a154 
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>
@timsaucer @claude
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>
@timsaucer @claude
Use ScalarUDFImpl::downcast_ref and is() instead of as &dyn Any patterns
3fa1869 
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>
@timsaucer @claude
Use AggregateUDFImpl::downcast_ref and is() instead of as &dyn Any pa…
fa7fa2d 
…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>
@timsaucer @claude
Use WindowUDFImpl::downcast_ref and is() instead of as &dyn Any patterns
02457bd 
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>
@github-actions github-actions bot added physical-expr Changes to the physical-expr crates functions Changes to functions implementation labels Apr 1, 2026
timsaucer
timsaucer commented Apr 1, 2026
View reviewed changes
datafusion/physical-expr/src/scalar_function.rs
pub fn try_downcast_func<T>(expr: &dyn PhysicalExpr) -> Option<&ScalarFunctionExpr>
where
T: 'static,
T: ScalarUDFImpl,
Copy link
Copy Markdown
Member Author

@timsaucer timsaucer Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@timsaucer timsaucer marked this pull request as ready for review April 1, 2026 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

catalog Related to the catalog crate core Core DataFusion crate datasource Changes to the datasource crate documentation Improvements or additions to documentation ffi Changes to the ffi crate functions Changes to functions implementation logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Changes to the physical-expr crates physical-plan Changes to the physical-plan crate proto Related to proto crate substrait Changes to the substrait crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@timsaucer @alamb