timsaucer opened a new issue, #25717:
URL: https://github.com/apache/datafusion/issues/25717
### Describe the bug
`FFI_ExecutionPlan::new` (`datafusion/ffi/src/execution_plan.rs:333`) has a
shortcut to avoid double-wrapping a plan that is a bare echo of something that
already crossed the boundary:
```rust
pub fn new(mut plan: Arc<dyn ExecutionPlan>, runtime: Option<Handle>) ->
Self {
if let Some(plan) = plan.downcast_ref::<ForeignExecutionPlan>() {
return plan.plan.clone();
}
```
`plan.downcast_ref::<T>()` here is the inherent `dyn
ExecutionPlan::downcast_ref`
(`datafusion/physical-plan/src/execution_plan.rs:1180`) — the public,
`downcast_delegate`-aware helper, **not** `Any::downcast_ref`:
```rust
pub fn downcast_ref<T: ExecutionPlan>(&self) -> Option<&T> {
match self.downcast_delegate() {
Some(delegate) => delegate.downcast_ref::<T>(),
None => (self as &dyn Any).downcast_ref(),
}
}
```
`downcast_delegate` exists (added in #22557) so a transparent wrapper can
redirect *public* downcasts to its inner plan while keeping its own type an
implementation detail. `datafusion-tracing`'s `InstrumentedExec` opts in
exactly for that — it is the case #22557 was written for.
The shortcut wants something different: "is `plan`'s *own concrete type*
literally `ForeignExecutionPlan`, so I can hand back its original FFI struct
instead of re-wrapping." Because it asks through the delegating helper, any
wrapper that opts into `downcast_delegate` and happens to wrap a
`ForeignExecutionPlan` is misclassified as *being* one — and the shortcut
returns the plan from **before the wrapper was applied**, discarding it.
This is a consumer of the `downcast_delegate` API misusing it at one call
site, not a problem with the API.
#### Three affected sites, not one
`dyn ExecutionPlan::is<T>()` is delegate-aware too
(`datafusion/physical-plan/src/execution_plan.rs:1163`):
```rust
pub fn is<T: ExecutionPlan>(&self) -> bool {
match self.downcast_delegate() {
Some(delegate) => delegate.is::<T>(),
None => (self as &dyn Any).is::<T>(),
}
}
```
so `pass_runtime_to_children` makes the same classification twice:
| site | code | effect on a transparent wrapper |
|---|---|---|
| `execution_plan.rs:337` | `plan.downcast_ref::<ForeignExecutionPlan>()` |
wrapper discarded, original foreign plan returned |
| `execution_plan.rs:291` | `let plan_is_foreign =
plan.is::<ForeignExecutionPlan>();` | wrapper classified as a foreign *parent* |
| `execution_plan.rs:310` | `plan_is_foreign &&
!child.is::<ForeignExecutionPlan>()` | wrapper *child* classified as
already-foreign, so it is not re-wrapped to receive the runtime |
Only the first has a confirmed user-visible fault (below). The other two are
the same confusion and should at minimum be audited as part of the fix — each
wants "is this literally a `ForeignExecutionPlan`", which is an FFI-internal
identity question that the public, delegate-aware API is the wrong tool for.
### To Reproduce
Verified against `datafusion-ffi` 55.1.0 by instrumenting local patched
copies of `datafusion-ffi`, `datafusion-physical-plan` and `datafusion-tracing`
with `eprintln!` at the relevant call sites, and running a two-node plan
(`ProjectionExec` over `DataSourceExec`) through `datafusion-tracing`'s
`instrument_with_info_spans!` rule, installed via `datafusion-python`'s
`SessionContext.add_physical_optimizer_rule`.
Direct evidence at the shortcut itself — a single call, two facts printed
side by side:
```text
plan.downcast_ref::<ForeignExecutionPlan>().is_some() == true
(plan as &dyn Any).type_id() ==
TypeId(0x36d0e1a1b79082b531906aa6608a769e) // InstrumentedExec
TypeId::of::<ForeignExecutionPlan>() ==
TypeId(0x585a73a9c18b03d9190859e857a74dfa) // different!
```
`Any`'s real type identity says `InstrumentedExec`; the delegating
`downcast_ref` used by the shortcut says `ForeignExecutionPlan` anyway, and the
shortcut fires.
Consequence: `ctx.add_physical_optimizer_rule(<datafusion-tracing's rule,
via FFI>)` followed by `ctx.sql(...).explain()` shows the completely
unmodified, un-instrumented plan — no `InstrumentedExec`, no
`FFI_ExecutionPlan:` wrapper, nothing — and the query executes with zero spans
created.
Wrapping the rule's output in one additional real,
non-`downcast_delegate`-opted-in node (e.g. `GlobalLimitExec::new(instrumented,
0, None)`, a no-op) works around it: `explain()` then shows `FFI_ExecutionPlan:
GlobalLimitExec` wrapping the correctly-transparent instrumented tree, and
spans are created and exported normally.
**Suggested in-tree repro**, following #24722's `import_as_foreign!`
pattern: build a native plan, force it foreign via
`crate::mock_foreign_marker_id`, convert it to a `ForeignExecutionPlan` via
`TryFrom<&FFI_ExecutionPlan>`, wrap *that* in a minimal test type whose
`downcast_delegate` returns the inner plan, and assert that
`FFI_ExecutionPlan::new` on the wrapper does **not** return `plan.plan.clone()`.
### Expected behavior
A transparent wrapper `ExecutionPlan` — any type implementing
`downcast_delegate`, not only `datafusion-tracing`'s — survives being handed
back across the boundary, instead of being silently unwound to the plan from
before it was applied.
### Proposed fix
The shortcut needs FFI-internal identity, which is exactly what raw `Any`
gives:
```rust
if let Some(plan) = (plan.as_ref() as &dyn
std::any::Any).downcast_ref::<ForeignExecutionPlan>() {
return plan.plan.clone();
}
```
One line, backward compatible — it only *narrows* when the shortcut fires —
independent of everything else, and needs no ABI bump. Apply the same treatment
to the two `is::<ForeignExecutionPlan>()` sites in `pass_runtime_to_children`
after confirming the intended semantics at each.
Worth auditing the other `FFI_*::new`-style "already foreign" fast paths
while this is fresh — see #24722, the same *family* of bug (an "already
foreign, take the shortcut" fast path misbehaving) for
`FFI_LogicalExtensionCodec`, `FFI_PhysicalExtensionCodec` and
`FFI_TableProvider`, though those discard constructor *arguments* rather than a
wrapper node.
### Related issues
* #25155 — physical optimizer rules cross the FFI boundary as handles
instead of bytes. Related; split from the same original report.
* #22557 — added `downcast_delegate`, precisely for the
`datafusion-tracing`/`InstrumentedExec` case this breaks.
* #24722 (closed) — same family: an "already foreign, take the shortcut"
fast path misbehaving in an FFI constructor, for
`FFI_LogicalExtensionCodec`/`FFI_PhysicalExtensionCodec`/`FFI_TableProvider`.
* #17374 — Stabilize FFI Boundary.
Part of umbrella #25152.
Downstream tracking: apache/datafusion-python#1739 (found via
`add_physical_optimizer_rule` trying to install
`datafusion-contrib/datafusion-tracing`'s instrumentation rule from Python).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
