You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "namanjain24-sudo (via GitHub)" <gi...@apache.org> on 2026/09/23 07:02:56 UTC
[PR] fix: Check Any, not the delegating downcast, in FFI_ExecutionPlan::new's foreign-echo shortcut [datafusion]
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Check Any, not the delegating downcast, in FFI_ExecutionPlan::new's foreign-echo shortcut [datafusion]
Posted by "namanjain24-sudo (via GitHub)" <gi...@apache.org>.
namanjain24-sudo commented on PR #25645:
URL: https://github.com/apache/datafusion/pull/25645#issuecomment-5853436919
Closing in favor of #25749, which does the same `FFI_ExecutionPlan::new` fix plus the `pass_runtime_to_children` fix this PR didn't cover, and is already approved.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Check Any, not the delegating downcast, in FFI_ExecutionPlan::new's foreign-echo shortcut [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25645:
URL: https://github.com/apache/datafusion/pull/25645#issuecomment-5791423359
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25645?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `30.64516%` with `43 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.44%. Comparing base ([`7570366`](https://app.codecov.io/gh/apache/datafusion/commit/7570366fd929daf9ced744bb8397686b50565b18?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c570149`](https://app.codecov.io/gh/apache/datafusion/commit/c570149f8dbb4de95f109ea1a754cad5b9c9d3d9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25645?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/ffi/src/execution\_plan.rs](https://app.codecov.io/gh/apache/datafusion/pull/25645?src=pr&el=tree&filepath=datafusion%2Fffi%2Fsrc%2Fexecution_plan.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9mZmkvc3JjL2V4ZWN1dGlvbl9wbGFuLnJz) | 30.64% | [41 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25645?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25645 +/- ##
==========================================
- Coverage 82.45% 82.44% -0.02%
==========================================
Files 1140 1140
Lines 436589 436650 +61
Branches 436589 436650 +61
==========================================
+ Hits 359996 360001 +5
- Misses 54838 54889 +51
- Partials 21755 21760 +5
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25645?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Check Any, not the delegating downcast, in FFI_ExecutionPlan::new's foreign-echo shortcut [datafusion]
Posted by "ShayanGho (via GitHub)" <gi...@apache.org>.
ShayanGho commented on PR #25645:
URL: https://github.com/apache/datafusion/pull/25645#issuecomment-5852514256
@namanjain24-sudo Thanks for working on this.
The constructor change looks correct, and all 120 FFI unit tests passed locally. #25717 also identifies the delegate-aware child check in pass_runtime_to_children, which remains unchanged here; the new test doesn’t exercise runtime propagation.
Since #25749 covers both checks, could we coordinate the overlapping PRs and clarify which one should land?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Check Any, not the delegating downcast, in FFI_ExecutionPlan::new's foreign-echo shortcut [datafusion]
Posted by "namanjain24-sudo (via GitHub)" <gi...@apache.org>.
namanjain24-sudo closed pull request #25645: fix: Check Any, not the delegating downcast, in FFI_ExecutionPlan::new's foreign-echo shortcut
URL: https://github.com/apache/datafusion/pull/25645
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org