Support transparent ExecutionPlan downcasts

[geoffreyclaude]

Which issue does this PR close?

• Closes ExecutionPlan downcasting no longer supports transparent wrapper nodes #22557.

Rationale for this change

DataFusion 54 changed ExecutionPlan downcasting to use the Any supertrait directly. That removes ExecutionPlan::as_any, which had also served as a customization point for wrapper nodes: wrappers could identify as themselves internally while exposing the wrapped plan type to normal downcast-based inspection.

This draft PR proposes one possible fix: add an explicit ExecutionPlan::downcast_delegate() hook for wrapper nodes that want their public ExecutionPlan downcast identity to be delegated to another plan.

The proposed behavior intentionally preserves the old as_any override semantics: when a node opts into downcast delegation, intermediate delegating wrappers are invisible to dyn ExecutionPlan::is::<T>() and downcast_ref::<T>(). A different "dual identity" model, where wrappers can downcast both as themselves and as their delegates, may also be useful, but that would be a new behavior rather than a compatibility fix.

This is only a suggested implementation for the linked issue. Other solution proposals, including different API names or a different shape for the hook, are very welcome.

What changes are included in this PR?

• Adds ExecutionPlan::downcast_delegate() with a default implementation returning None.
• Updates dyn ExecutionPlan::is::<T>() and downcast_ref::<T>() to delegate to downcast_delegate() when present, otherwise use the current concrete plan type.
• Documents that downcast_delegate() is only for type introspection and is independent from children() / plan traversal.
• Adds tests for direct and nested downcast-delegating wrappers, including that intermediate delegating wrappers remain invisible to normal downcast-based inspection.

An alternative API shape would make every plan expose an explicit downcast target. A concrete spelling could make the self-target case explicit:

enum DowncastTarget<'a> {
    SelfTarget,
    Plan(&'a dyn ExecutionPlan),
}

fn downcast_target(&self) -> DowncastTarget<'_> {
    DowncastTarget::SelfTarget
}

pub fn downcast_ref<T: ExecutionPlan>(&self) -> Option<&T> {
    match self.downcast_target() {
        DowncastTarget::Plan(target) => target.downcast_ref::<T>(),
        DowncastTarget::SelfTarget => (self as &dyn Any).downcast_ref(),
    }
}

That frames every plan as having a public downcast target, which is close to the old as_any mental model. A simpler conceptual version would be fn downcast_target(&self) -> &dyn ExecutionPlan with the default target being self, but the real helper still needs an explicit self-target base case to avoid recursing forever. This PR keeps the primary proposal as an explicit Option-based opt-in.

Are these changes tested?

Yes.

• cargo test -p datafusion-physical-plan execution_plan_downcast
• cargo test -p datafusion-physical-plan --lib
• cargo fmt --all -- --check
• git diff --check

Are there any user-facing changes?

Yes. This adds a new public ExecutionPlan trait method with a default implementation, and it changes ExecutionPlan downcast helpers to honor wrappers that explicitly opt into delegating public downcast identity.

[timsaucer]

I'll review as soon as it's marked ready, but at a high level looks good so far

[geoffreyclaude]

I'll review as soon as it's marked ready, but at a high level looks good so far

@timsaucer Should be ready for review now. I just renamed the API to downcast_delegate which seemed clearer to me.

[gabotechs]

👍 Even if it's not super pretty to have that extra method there, it gets the job done without introducing any breaking API change, and it's landed in a way that only people caring about implementing wrapper types need to care about this, without getting in the way of everyone else, so +1 on my side.

Before moving forward, let's wait for at least @timsaucer to give his opinion here.

[timsaucer]

Won't this prevent you from downcasting to the wrapper type?

[geoffreyclaude]

Yes, and this is intentional to mimic the old as_any behavior. I've updated the PR description to be clear about this choice.

[timsaucer]

It seems like the earlier PR would still work for all use cases, right? Because you're always choosing to downcast to a specific type, it seems like it would enable downcasting to either (or many, depending on levels) type.

[geoffreyclaude]

The previous PR would stop on the first match, rather than go all the way to the leaf child. I agree this is being pedantic, as "real world" cases would probably never have a case where you'd need to match the leaf child but get interrupted by an intermediate node in a "wrapper chain". However, I still find the current implementation clearer and easier to reason about: the leaf node is what is returned.

[alamb]

Should we revert the hew :Any instead? I think it was just supposed to be a cleanup but we didn't realize it would have downstream impacts 🤔

[alamb]

Also I wonder if there is some way to avoid using as_any / delegating logic at all ... But to do that we would have to remove all uses of as_any / downcasting I suspect

[geoffreyclaude]

Should we revert the new :Any instead? I think it was just supposed to be a cleanup but we didn't realize it would have downstream impacts 🤔

@alamb Honestly I prefer the new API with downcast_delegate: it's explicit and clearer.

Also I wonder if there is some way to avoid using as_any / delegating logic at all ... But to do that we would have to remove all uses of as_any / downcasting I suspect

That would be a pretty big breaking change as well, as the is a check is quite widespread.

[gene-bordegaray]

looks good

I think this is feasible and good as we are gonna get 👍

[timsaucer]

Do we need to also open a PR to target branch-54?

[geoffreyclaude]

Do we need to also open a PR to target branch-54?

@timsaucer I opened #22565 on branch-54, which is a clean cherry-pick of this PR's single commit. I suggest we give both at least 24 hours grace before merging, so others can chime in if needed.

[alamb]

Should we revert the hew :Any instead? I think it was just supposed to be a cleanup but we didn't realize it would have downstream impacts 🤔

[alamb]

Also I wonder if there is some way to avoid using as_any / delegating logic at all ... But to do that we would have to remove all uses of as_any / downcasting I suspect

[alamb]

It might be good to add a specific example here (like a wrapper node that wants to observe changes but still be treated as the underlying node when DataFusion checks for patterns using downcast 🤔

[geoffreyclaude]

There's the unit test with DowncastDelegatingExec which already shows a possible implementation. Would you also like an inline comment?

[alamb]

Yeah, I was thinking an inline comment so that if someone else is reading (just) the ExecutionPlan docs they can more quickly understand if they need to worry about implementing this method or not (probably not)

[geoffreyclaude]

done: I amended the commit to be clearer in the comment.

[alamb]

Looks good to me -- thank you everyone