Skip to content

fix(ffi): prevent recursive session physical planning - #24492

Open
goutamadwant wants to merge 2 commits into
apache:mainfrom
goutamadwant:fix-ffi-session-plan-recursion
Open

fix(ffi): prevent recursive session physical planning#24492
goutamadwant wants to merge 2 commits into
apache:mainfrom
goutamadwant:fix-ffi-session-plan-recursion

Conversation

@goutamadwant

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

ForeignSession::create_physical_plan forwarded through the owning session. If that session had installed the calling library's query planner, the callback re-entered the same planner and could recurse until the stack was exhausted. The callback also returned an FFI_ExecutionPlan, which cannot reconstruct built-in nodes with the receiving library's Rust type identities for downcasting.

The supported delegation boundary is FFI_QueryPlanner: the session owner exports its original planner before installing a foreign planner, and the foreign planner retains that handle. This boundary serializes physical plans and reconstructs them with local type identities.

What changes are included in this PR?

  • Remove the unsafe create_physical_plan callback from FFI_SessionRef.
  • Make ForeignSession::create_physical_plan return an actionable NotImplemented error without invoking the installed planner.
  • Document the captured-FFI_QueryPlanner migration in the module documentation and DataFusion 55 upgrade guide.
  • Add a regression test that verifies the installed planner is not invoked through the unsupported session method.

Draft PR #24108 contains broader extension-codec bundle work and currently updates the removed callback's logical-codec path. That callback-specific hunk will no longer be needed after this change; the remaining codec-bundle work is outside this PR.

Are these changes tested?

Yes.

  • The focused regression test passes and verifies zero planner invocations.
  • cargo test -p datafusion-ffi --features integration-tests
  • cargo clippy -p datafusion-ffi --all-targets --all-features -- -D warnings
  • cargo clippy --all-targets --all-features -- -D warnings
  • RUST_BACKTRACE=1 cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-cli --workspace --lib --tests --bins --features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption
  • RUSTDOCFLAGS="-D warnings" cargo doc -p datafusion-ffi --all-features --no-deps
  • cargo fmt --all -- --check
  • ./ci/scripts/doc_prettier_check.sh

The cross-library query-planner tests continue to verify that the captured planner route restores local, downcastable physical-plan nodes.

Are there any user-facing changes?

Yes. ForeignSession::create_physical_plan now returns NotImplemented; callers must retain and invoke the session owner's exported FFI_QueryPlanner. Removing the callback changes the FFI_SessionRef ABI layout, so FFI producers and consumers must be rebuilt against DataFusion 55. This PR targets main, must not be backported to a patch-release branch, and requires the api change label.

@github-actions github-actions Bot added documentation Improvements or additions to documentation ffi Changes to the ffi crate labels Aug 19, 2026
@codecov-commenter

codecov-commenter commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.46154% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.38%. Comparing base (5134a1a) to head (2039e19).

Files with missing lines Patch % Lines
datafusion/ffi/src/session/mod.rs 88.46% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24492      +/-   ##
==========================================
- Coverage   81.38%   81.38%   -0.01%     
==========================================
  Files        1116     1116              
  Lines      397960   397963       +3     
  Branches   397960   397963       +3     
==========================================
- Hits       323880   323879       -1     
- Misses      55120    55127       +7     
+ Partials    18960    18957       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation ffi Changes to the ffi crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FFI Session::create_physical_plan could recurse infinitely

2 participants