namanjain24-sudo opened a new pull request, #25645:
URL: https://github.com/apache/datafusion/pull/25645
related: #25155
This addresses only "Direction 3" (the `FFI_ExecutionPlan::new` shortcut fix)
from that issue, using the exact change the issue itself proposes. The issue
also covers a separate, larger problem ("Direction A") that needs its own
design discussion and is not touched here, so this should not close it.
## What happened
`FFI_ExecutionPlan::new` short-circuits when the incoming plan is already a
`ForeignExecutionPlan`, handing back its original FFI handle instead of
re-wrapping it:
```rust
if let Some(plan) = plan.downcast_ref::<ForeignExecutionPlan>() {
return plan.plan.clone();
}
```
That check goes through `ExecutionPlan::downcast_ref`, which follows
`downcast_delegate` when a plan opts in (added in #22557 so a transparent
wrapper can redirect public downcasts to its inner plan). A wrapper that
delegates to a `ForeignExecutionPlan` is therefore misclassified as being
one, and the shortcut discards the wrapper, returning the plan from before
it was ever applied.
## What changes are included in this PR
Switch the check to a raw `Any` downcast, which only matches `plan`'s own
concrete type rather than what `downcast_delegate` redirects to:
```rust
if let Some(plan) = (plan.as_ref() as &dyn
std::any::Any).downcast_ref::<ForeignExecutionPlan>() {
return plan.plan.clone();
}
```
For a plan whose own concrete type really is `ForeignExecutionPlan` (which
does not implement `downcast_delegate`), both checks agree, so this only
narrows the shortcut for the wrapper case. No ABI change.
## What is the testing strategy for this PR
- New unit test in `datafusion/ffi/src/execution_plan.rs`: a minimal
`downcast_delegate`-opted-in wrapper around a plan mocked as foreign (the
existing `mock_foreign_marker_id` pattern already used by sibling tests in
this file) must survive `FFI_ExecutionPlan::new`. Verified it fails
against the unpatched check — the wrapper is discarded and the round trip
reports the inner plan's name instead of the wrapper's — and passes with
the fix.
- `cargo test -p datafusion-ffi --lib` (120 tests) and every cross-library
integration test binary under `--features integration-tests`
(`ffi_execution_plan`, `ffi_query_planner`, `ffi_physical_optimizer`,
`ffi_integration`, `ffi_udaf`, `ffi_udf`, `ffi_udtf`, `ffi_udwf`,
`ffi_catalog`, `ffi_config`; 31 tests), all passing.
- `cargo fmt --check` and `cargo clippy -p datafusion-ffi --all-targets
--all-features` report nothing for the changed file.
## Are there any user-facing changes?
A transparent `downcast_delegate` wrapper around a plan that already crossed
the `datafusion-ffi` boundary is no longer silently discarded by
`FFI_ExecutionPlan::new`. No public API changes.
--
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]
