Consider `pub use instrumented::InstrumentedExec`

[xudong963]

The InstrumentedExec itself is pub, but instrumented isn't pub in lib.rs.

It would be helpful to pub use instrumented::InstrumentedExec

cc @geoffreyclaude

[geoffreyclaude]

@xudong963: I didn't make it pub on purpose to avoid users matching on the concrete type and modifying the execution plan after instrumentation, as well as to enforce it being the "last" optimization rule. If you have a particular use-case in mind that needs pub, I'd be happy to expose it, otherwise I think it's safer to keep it hidden (and maybe add a clarifying comment as to why it is so.)

[Allan-Clements]

Hi @geoffreyclaude I'm one of @xudong963's co-workers. I can give some more context to what prompted opening this ticket.

We have InstrumentRule being set as the last rule that should run. That way its presence is hidden from some of our other rules. However InstrumentedExec works by delegating almost everything to what it wraps works so long as you don't have rules that depend on inspecting implementation details of the type behind the delegated trait, which we do and usually downcast_ref to the underlying type, but because InstrumentedExec isn't visible outside the crate once it wraps a plan node we can't downcast through it.

Ideally with it being the last rule this shouldn't be a problem. Except we noticed with something we have that wraps a ViewTable and it does its own logical plan -> physical plan evaluation which then returns plan nodes wrapped with InstrumentedExec earlier than expected causing rules that run after that nested evaluation to then come in contact with InstrumentedExec.

For what it's worth this is a maneuver that is done in datafusion as well. Example. So there may be wider spread consequences than just this instance we've bumped in to.

[geoffreyclaude]

Thanks for the detailed explanation, it makes more sense now. It seems to me that exposing InstrumentedExec would be more of a workaround than a proper solution.
There was a very similar issue to yours (if I understand correctly!) regarding recursive queries, which I solved with apache/datafusion#16469 . Do you think this would help with your particular case?
To me, matching on an execution node's concrete type is not safe in general as nothing guarantees it won't have been replaced or wrapped by some optimization rule or other process.
But if you think making InstrumentedExec public is the only solution, we can of course go that direction.

[Allan-Clements]

@geoffreyclaude it looks like it could be an option. Thanks! Unfortunately we're still targeting DF47 as of right now, and if I'm reading correctly it looks like that change was after DF48 was released.

So maybe we'll take another swing at this in DF49 & later. But good to know there's a potential path forward in the future.

and maybe add a clarifying comment as to why it is so.

Probably not a bad idea fwiw, I imagine others may bump into this too and hopefully do their due diligence and try to find this issue before opening another one, but maybe they won't 😅 .

[geoffreyclaude]

@Allan-Clements What would be super helpful is if you could write a reproducer for your View issue, similar to what was done in #5 . Like that I can give it a more in depth look and see if my above suggestion is relevant or not.