fix(ffi): prevent recursive session physical planning - #24492
Open
goutamadwant wants to merge 2 commits into
Open
fix(ffi): prevent recursive session physical planning#24492goutamadwant wants to merge 2 commits into
goutamadwant wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Session::create_physical_plancould recurse infinitely #24065.Rationale for this change
ForeignSession::create_physical_planforwarded 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 anFFI_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?
create_physical_plancallback fromFFI_SessionRef.ForeignSession::create_physical_planreturn an actionableNotImplementederror without invoking the installed planner.FFI_QueryPlannermigration in the module documentation and DataFusion 55 upgrade guide.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.
cargo test -p datafusion-ffi --features integration-testscargo clippy -p datafusion-ffi --all-targets --all-features -- -D warningscargo clippy --all-targets --all-features -- -D warningsRUST_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_encryptionRUSTDOCFLAGS="-D warnings" cargo doc -p datafusion-ffi --all-features --no-depscargo fmt --all -- --check./ci/scripts/doc_prettier_check.shThe 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_plannow returnsNotImplemented; callers must retain and invoke the session owner's exportedFFI_QueryPlanner. Removing the callback changes theFFI_SessionRefABI layout, so FFI producers and consumers must be rebuilt against DataFusion 55. This PR targetsmain, must not be backported to a patch-release branch, and requires theapi changelabel.