Commit 96ff37a
authored
fix(proto): honor ExecutionPlan downcast_delegate during serialization (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.1 parent 994b926 commit 96ff37a
2 files changed
Lines changed: 71 additions & 3 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
442 | 442 | | |
443 | 443 | | |
444 | 444 | | |
445 | | - | |
| 445 | + | |
446 | 446 | | |
447 | 447 | | |
448 | 448 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
89 | 89 | | |
90 | 90 | | |
91 | 91 | | |
92 | | - | |
93 | | - | |
| 92 | + | |
| 93 | + | |
94 | 94 | | |
95 | 95 | | |
96 | 96 | | |
| |||
229 | 229 | | |
230 | 230 | | |
231 | 231 | | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
232 | 300 | | |
233 | 301 | | |
234 | 302 | | |
| |||
0 commit comments