fix: carry input requirements across FFI_ExecutionPlan by andygrove · Pull Request #27428 · apache/datafusion

andygrove · 2026-11-14T10:34:56Z

Which issue does this PR close?

Closes FFI_ExecutionPlan loses input distribution and ordering requirements, so optimizer rules across FFI produce invalid plans #27427.

Rationale for this change

A ForeignExecutionPlan reports the ExecutionPlan defaults for its input requirements (UnspecifiedDistribution, no required ordering, maintains_input_order = false), whatever the wrapped plan requires. When a physical optimizer rule runs on the other side of the FFI boundary from the plan it optimizes, it can't see those requirements. That happens with a foreign QueryPlanner whose session rules belong to the host. EnsureRequirements then puts only a round-robin repartition under a FinalPartitioned aggregate, which can split one group key across several partitions.

For FFI query planner users this is a regression from 55 to 56: a query that works on 55.0.0 fails at planning time on 56. Until #26461, SanityCheckPlan ran as a host-side rule and was just as blind, so the plan got through (correct by luck in the affected test, unsafe in general). Since #26461 the check runs inside DefaultPhysicalPlanner and rejects it. This is the only failure in the datafusion-python upgrade to 56 (apache/datafusion-python#1826). The issue has the full error, the plan and the 55 → 56 history.

What changes are included in this PR?

FFI_ExecutionPlan gets four callbacks: input_distribution_requirements, required_input_ordering, maintains_input_order and benefits_from_input_partitioning. ForeignExecutionPlan forwards them, and also overrides the deprecated required_input_distribution so it agrees.
New stable types:
- FFI_Distribution and FFI_InputDistributionRequirements in physical_expr/partitioning.rs. Co-partitioning crosses as a bool, because InputDistributionRequirements can only be built with all children co-partitioned or none.
- FFI_PhysicalSortRequirement and FFI_OrderingRequirements (hard/soft alternatives) in physical_expr/sort.rs.
The deprecated Distribution::HashPartitioned crosses as KeyPartitioned. The optimizer treats the two the same, and this keeps the deprecated variant out of the ABI.
A note in the 57.0.0 upgrade guide.

What is the testing strategy for this PR?

test_ensure_requirements_sees_foreign_plan_requirements reproduces the bug. It plans SELECT a % 2, count(*) ... GROUP BY 1 with target_partitions = 4, removes the hash repartition, sends the FinalPartitioned aggregate across a mock FFI boundary, and runs EnsureRequirements. It asserts that a hash repartition is inserted and that check_plan_tree_sanity passes. Without the fix it fails: the plan gets RoundRobinBatch(4).
test_ffi_execution_plan_input_requirements_round_trip: default and non-default requirements, including co-partitioning, soft ordering and the deprecated per-child accessor, come back unchanged from a foreign plan.
Unit round-trip tests for the new FFI types in partitioning.rs and sort.rs.
End to end: I cherry-picked this onto the branch-56 commit Upgrade DataFusion to version 56 datafusion-python#1826 builds against. The previously failing test_spawning_plan_across_three_libraries now passes, along with the rest of that repo's FFI example suites.

Are there any user-facing changes?

The FFI_ExecutionPlan layout changes, so FFI providers and consumers need to be rebuilt against the same release. Foreign plans now report their real input requirements to optimizer rules.

Because of the datafusion-python 56 upgrade, this may be worth backporting to branch-56. The cherry-pick applies cleanly apart from the upgrade-guide file.

🤖 Generated with Claude Code

ForeignExecutionPlan fell back to the ExecutionPlan defaults for input_distribution_requirements, required_input_ordering, maintains_input_order and benefits_from_input_partitioning. Physical optimizer rules running on the other side of the boundary, such as a foreign QueryPlanner's session rules, could not see what the plan required: EnsureRequirements added only a round-robin repartition under a FinalPartitioned aggregate. Add FFI callbacks and stable representations for Distribution, InputDistributionRequirements, PhysicalSortRequirement and OrderingRequirements, and forward them from ForeignExecutionPlan. Closes apache#27427 Co-Authored-By: Claude Opus 5.5 <[email protected]>

viirya

Thanks for tracking this down. The four callbacks are the right set: every other method ForeignExecutionPlan doesn't forward has a conservative default.

Two things before merging:

Backport target. version() reports only the major version, and the new fields shift clone/release/version. Shipping this in a 56.0.x patch would leave 56.0.0 and 56.0.x libraries with different layouts and no way to detect the mismatch. Since 56 is still at rc1, can this target 56.0.0 instead, with the upgrade note in the 56 guide?
Test coverage of foreign expressions. See the inline comment on the EnsureRequirements test. The cross-library path compares expressions by identity, and the test doesn't reach that path.

viirya · 2026-11-14T17:40:05Z

+                    ReplaceChildrenOptions::new(ChildrenPropertiesMode::Recompute),
+                )?;
+                let mut ffi = FFI_ExecutionPlan::new(stripped, None);
+                ffi.library_marker_id = crate::mock_foreign_marker_id;


Only the plan gets the mock marker. The expressions inside the requirements are created with the real marker, so they come back as native Columns. With a real library boundary they become ForeignPhysicalExpr, whose eq is ptr::eq, and every call to these callbacks builds new wrappers. On the host, a foreign requirement then never matches an existing partitioning or ordering. The result is still correct, but it can add repartitions or sorts that native planning wouldn't. Does the datafusion-python EXPLAIN show any? For example, an aggregate over a join on the same key, or a window over sorted input. If it does, could you open a follow-up issue, or add a test in ffi_integration that crosses a real cdylib?

viirya · 2026-11-14T17:40:05Z

+        self.input_distribution_requirements().into_per_child()
+    }
+
+    fn input_distribution_requirements(&self) -> InputDistributionRequirements {


These are called many times per node during EnsureRequirements/EnforceSorting, and each call crosses FFI and rebuilds the expression wrappers. Requirements are fixed for a plan instance. Could they be converted once in TryFrom<FFI_ExecutionPlan>, the way properties is?

viirya · 2026-11-14T17:40:05Z

+                FFI_Option::Some(ordering) => OrderingRequirements::try_from(ordering)
+                    .inspect_err(|e| {
+                        log::error!(
+                            "Invalid required input ordering from foreign plan {}: {e}",


Returning None here drops a hard ordering requirement. As far as I can tell this can't happen with a well-formed provider, because LexRequirement and OrderingRequirements are non-empty by construction. Could a comment say so, so the fallback doesn't read as a safe default?

viirya · 2026-11-14T17:40:05Z

+    /// One distribution per child.
+    distributions: SVec<FFI_Distribution>,
+    /// Whether all children must be co-partitioned.
+    co_partitioned: bool,


nit: InputDistributionRequirements stores co-partitioned child indexes and is #[non_exhaustive]. A short note that the bool relies on all-or-none construction would flag this spot if subsets become possible.

viirya · 2026-11-14T17:40:05Z

            unimplemented!()
        }

+        fn required_input_distribution(&self) -> Vec<Distribution> {


nit: The test never reaches this override, because ForeignExecutionPlan::required_input_distribution calls its own input_distribution_requirements. Can it be removed?

github-actions Bot added documentation Improvements or additions to documentation ffi Changes to the ffi crate labels Nov 14, 2026

andygrove mentioned this pull request Nov 14, 2026

Upgrade DataFusion to version 56 apache/datafusion-python#1826

Open

viirya reviewed Nov 14, 2026

View reviewed changes

Navigation Menu

Repository navigation

fix: carry input requirements across FFI_ExecutionPlan - #27428

fix: carry input requirements across FFI_ExecutionPlan#27428
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:ffi-plan-input-requirements

andygrove commented Nov 14, 2026

Uh oh!

viirya left a comment

Uh oh!

viirya Nov 14, 2026

Uh oh!

viirya Nov 14, 2026

Uh oh!

viirya Nov 14, 2026

Uh oh!

viirya Nov 14, 2026

Uh oh!

viirya Nov 14, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

andygrove commented Nov 14, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

What is the testing strategy for this PR?

Are there any user-facing changes?

Uh oh!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

viirya Nov 14, 2026

Choose a reason for hiding this comment

Uh oh!

viirya Nov 14, 2026

Choose a reason for hiding this comment

Uh oh!

viirya Nov 14, 2026

Choose a reason for hiding this comment

Uh oh!

viirya Nov 14, 2026

Choose a reason for hiding this comment

Uh oh!

viirya Nov 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants