jayzhan211 opened a new pull request, #26461:
URL: https://github.com/apache/datafusion/pull/26461
## Which issue does this PR close?
- Closes #26460.
Related: #26171 (option 3), #26351, #26360, #25688.
## Rationale for this change
`SanityCheckPlan` is the last rule in `PhysicalOptimizer::new()`, so rules
appended with `SessionStateBuilder::with_physical_optimizer_rule` run after it
and are never checked. On `main`, an appended rule that removes a window's sort
silently returns wrong `row_number()` values. Custom pipelines have to keep the
rule last by hand, and a custom list without it runs plans whose requirements
do not hold. When the check does fail, the error only says `SanityCheckPlan`,
so finding the rule that broke the plan has needed a manual bisect each time
(for example #26391, #17077, #25921).
#26171 agreed (option 3) that a custom pipeline must keep the plan valid,
while optimization coverage is not guaranteed. This PR makes that validity part
checked for every pipeline. The review on #26360 asked for the same shape: one
rule, `EnsureRequirements`, makes the plan valid, and every later rule keeps it
valid.
## What changes are included in this PR?
- `SanityCheckPlan` leaves the default rule list.
`DefaultPhysicalPlanner::optimize_physical_plan` runs the same check (unbounded
inputs, ordering, distribution, `check_invariants(Executable)`) after the last
rule, including appended rules. It replaces the planner's own
`InvariantChecker(Executable)` walk, which `SanityCheckPlan` repeated.
- The planner keeps the plan after each rule (one `Arc` per rule). When the
ordering or distribution requirements fail, the error names the rule that broke
them:
```text
SanityCheckPlan: order and distribution requirements stopped holding after
physical optimizer rule 'ProjectionPushdown'
caused by
Error during planning: Plan: [...] does not satisfy order requirements:
[...]
```
or, when the plan was already invalid as `EnsureRequirements` produced it:
```text
SanityCheckPlan: the plan produced by physical optimizer rule
'EnsureRequirements' does not satisfy the order and distribution requirements
```
Plans before `EnsureRequirements` need not satisfy the requirements, so
only plans from its last run onwards are searched. The rule is recognized by
its name; a separate enforcement phase (#25688) would give a cleaner boundary.
Errors about unbounded inputs keep the plain `SanityCheckPlan` context, so
existing messages do not change.
- `sanity_checker` exposes `check_plan_tree_sanity`,
`check_input_requirements` and `input_requirements_hold`, which the planner
uses.
- The upgrade guide, the optimizer rule reference and `explain.slt` are
updated.
## What is the testing strategy for this PR?
- `sanity_checker.rs`: a rule appended after the default rules that removes
sorts is reported by name, both with the default list and with a custom list; a
list without `EnsureRequirements` reports that no rule produced a valid plan;
the default list passes.
- `memory_limit/mod.rs`: the two sort merge join tests ran a
`SortMergeJoinExec` over unsorted streams through a custom rule list without
`EnsureRequirements`. They now sort the generated data by the join keys and
declare that order, so the plan is valid and the tests still measure the join's
memory use.
- The extended workspace suite passes.
## Are there any user-facing changes?
- Plans produced by rules added with `with_physical_optimizer_rule`, or by a
custom rule list, are now checked. A plan whose requirements do not hold, such
as a sort merge join over unsorted input, fails to plan instead of running and
possibly returning wrong results.
- `PhysicalOptimizer::new()` no longer contains `SanityCheckPlan`, and
`EXPLAIN VERBOSE` no longer prints `physical_plan after SanityCheckPlan`. Code
that looks the rule up in the default list no longer finds it.
`SanityCheckPlan` itself remains available.
- Errors for broken ordering or distribution requirements name the rule that
broke them.
This changes the public behavior of the default rule list, so it needs the
`api change` label.
--
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]
