Skip to content

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

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

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

Conversation

@goutamadwant

@goutamadwant goutamadwant commented Aug 19, 2026

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?

  • Retain the legacy create_physical_plan callback at its original position and signature in FFI_SessionRef for DataFusion 55 ABI compatibility, while making it return an actionable NotImplemented error without invoking the installed planner.
  • Make ForeignSession::create_physical_plan return the same error without crossing the FFI boundary or invoking the installed planner.
  • Document the captured-FFI_QueryPlanner migration in the module documentation and DataFusion 56 upgrade guide.
  • Add unit and cross-library regression coverage for the rejected direct-delegation path and the successful captured-planner path.

#24690 added the session's logical extension codec to the callback and fixed planning for logical plans that require custom codec support. It does not address the two remaining problems covered here: forwarding can re-enter the installed foreign planner, and the returned FFI_ExecutionPlan does not reconstruct built-in nodes with the receiving library's Rust type identities. This PR preserves the callback slot for ABI compatibility but makes direct session planning unsupported, so its callback-specific codec wiring is no longer exercised; other FFI codec work remains outside this PR.

Are these changes tested?

Yes.

  • The focused unit regression verifies that both ForeignSession::create_physical_plan and the retained legacy callback return NotImplemented with zero planner re-entry.
  • The three-library dlopen regression verifies that direct foreign-session delegation is rejected while the captured FFI_QueryPlanner route succeeds and restores local, downcastable physical-plan nodes.
  • 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

Are there any user-facing changes?

Yes. ForeignSession::create_physical_plan and the retained legacy callback now return NotImplemented; callers must retain and invoke the session owner's exported FFI_QueryPlanner. The FFI_SessionRef callback remains at its original ABI position and signature, so this PR does not change the struct layout or require consumers to rebuild solely because of an ABI layout change.

@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 91.42857% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.61%. Comparing base (35f58f5) to head (b66acf8).

Files with missing lines Patch % Lines
datafusion/ffi/src/session/mod.rs 91.42% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #24492   +/-   ##
=======================================
  Coverage   81.61%   81.61%           
=======================================
  Files        1124     1124           
  Lines      411978   411977    -1     
  Branches   411978   411977    -1     
=======================================
+ Hits       336236   336241    +5     
+ Misses      55936    55935    -1     
+ Partials    19806    19801    -5     

☔ 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.

@kosiew

kosiew commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

@goutamadwant
Can you resolve the merge conflicts?

@goutamadwant

Copy link
Copy Markdown
Contributor Author

@kosiew done. thanks!

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@goutamadwant,

Thanks for working on this. The planner delegation direction makes sense, but I found one ABI compatibility issue that I think needs to be addressed before this can land. I also left one non-blocking suggestion around strengthening the cross-library coverage.

logical_plan_serialized: SVec<u8>,
) -> FFI_Result<SVec<u8>>,

create_physical_plan:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to keep this callback slot for ABI compatibility. Removing it changes the #[repr(C)] FFI_SessionRef layout while the workspace is still at 55.0.0.

A separately compiled 55.x consumer would still interpret this old slot as create_physical_plan, so it could read create_physical_expr as that callback and every field after it would be shifted. That can result in function pointers being called with the wrong signatures, which is UB.

Could we keep the callback field in the struct and have its wrapper return the new NotImplemented error instead? The other option would be to treat this as an explicitly versioned ABI break and add compatible-version gating.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kosiew Addressed in latest changes. I restored create_physical_plan at its exact original position and signature in FFI_SessionRef, including initialization through the construction and clone paths. The retained callback now returns NotImplemented without invoking the installed planner, preserving the DataFusion 55 layout and preventing shifted function-pointer calls. Let me know if this is good now. thanks!

let batch = record_batch!(("a", Int32, [1, 2, 3]))?;
let table = MemTable::try_new(schema, vec![vec![batch]])?;
ctx.register_table("test_table", Arc::new(table))?;
async fn test_foreign_session_rejects_create_physical_plan() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we consider moving this assertion into the cross-library query-planner integration path, or add a small cross-library case for it? That would verify that a foreign planner gets the expected NotImplemented result when it tries direct session delegation, while the retained-planner path still works correctly across dlopen.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kosiew I extended the existing three-library dlopen planner-swap integration test to verify that direct foreign-session delegation returns DataFusionError::NotImplemented, then continues through the captured FFI_QueryPlanner and successfully reconstructs local, downcastable execution-plan nodes. The unit regression also invokes the retained callback directly and verifies zero planner re-entry. Hope this covers it. :)

@kosiew

kosiew commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

@goutamadwant
Can you resolve the merge conflicts?

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

3 participants