Conversation
0utsights
marked this pull request as ready for review
September 14, 2026 02:25
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25245 +/- ##
==========================================
- Coverage 81.90% 81.90% -0.01%
==========================================
Files 1134 1134
Lines 425217 425288 +71
Branches 425217 425288 +71
==========================================
+ Hits 348289 348341 +52
- Misses 56288 56299 +11
- Partials 20640 20648 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The FFI ABI change requires a major-version compatibility gate and human review.
Pull request overview
Preserves scheduling and evaluation metadata across the DataFusion FFI boundary.
Changes:
- Adds FFI enums and callbacks for scheduling/evaluation types.
- Restores both properties during foreign-plan reconstruction.
- Adds local and cross-library regression tests.
Blocking review note: The FFI layout changes without a corresponding major-version compatibility gate, risking ABI corruption for existing consumers.
File summaries
| File | Description |
|---|---|
datafusion/ffi/tests/ffi_execution_plan.rs |
Verifies metadata preservation and optimizer behavior across libraries. |
datafusion/ffi/src/tests/mod.rs |
Configures test plans with nondefault properties. |
datafusion/ffi/src/plan_properties.rs |
Implements metadata transport, reconstruction, and round-trip tests. |
datafusion/ffi/src/execution_plan.rs |
Adds test-plan property builders. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
timsaucer
reviewed
Sep 15, 2026
timsaucer
left a comment
timsaucer
left a comment
Member
There was a problem hiding this comment.
Looks pretty clean, just one question. I'll also let the AI take a look before hitting the approve. Thank you for the contribution.
Comment on lines
+598
to
+612
| pub fn with_scheduling_type( | ||
| mut self, | ||
| scheduling_type: datafusion_physical_plan::execution_plan::SchedulingType, | ||
| ) -> Self { | ||
| Arc::make_mut(&mut self.props).scheduling_type = scheduling_type; | ||
| self | ||
| } | ||
|
|
||
| pub fn with_evaluation_type( | ||
| mut self, | ||
| evaluation_type: datafusion_physical_plan::execution_plan::EvaluationType, | ||
| ) -> Self { | ||
| Arc::make_mut(&mut self.props).evaluation_type = evaluation_type; | ||
| self | ||
| } |
Member
There was a problem hiding this comment.
Was this all necessary? EmptyExec is just a test execution plan - is there any reason not to just set those properties by default?
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?
Closes #25153.
Rationale for this change
A foreign execution plan loses its scheduling and evaluation properties when converted into native
PlanProperties. A cooperative, eager producer becomes noncooperative and lazy on the consumer side, changing the metadata used byEnsureCooperative.What changes are included in this PR?
Add FFI enums and producer callbacks for
scheduling_typeandevaluation_type, and restore both values during foreign plan-property reconstruction. Preserve the existing local-marker shortcut and ownership/release paths.This is AI-assisted work and remains a draft pending the contributor's end-to-end review of the implementation and ABI implications.
What is the testing strategy for this PR?
Unit tests exercise every scheduling/evaluation variant through local and forced foreign conversions. A separate-library integration test checks both nondefault properties on a
ForeignExecutionPlanand verifies thatEnsureCooperativeleaves the already cooperative leaf unwrapped. The foreign regressions fail on the baseline and pass with the fix.Validation on Linux x86_64 with Rust 1.97.0:
integration-tests: 156 passed, including the separately loaded.soregression.AGENTS.mdpasses, including the core fuzz and SQL logic suites.The FFI integration suite also passes on Windows with a separately loaded DLL. Both sides use the same source/toolchain in each test; mixed-toolchain compatibility and a runtime speedup were not measured.
Are there any user-facing changes?
Foreign execution plans retain their producer's scheduling and evaluation metadata. This changes the
FFI_PlanPropertieslayout and requires downstream libraries to rebuild for the new major version. This PR targetsmainand must not be backported to a patch release. Please apply theapi changelabel before merge.