andygrove opened a new pull request, #27428:
URL: https://github.com/apache/datafusion/pull/27428
## Which issue does this PR close?
- Closes #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
apache/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](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
