jayzhan211 opened a new issue, #26460:
URL: https://github.com/apache/datafusion/issues/26460
### Is your feature request related to a problem or challenge?
`SanityCheckPlan` checks that every node's required input ordering and
distribution are satisfied, and that no pipeline-breaking operator runs on an
unbounded input. Because it is a rule at the end of `PhysicalOptimizer::new()`,
three things go wrong:
1. **Rules appended by the user are never checked.**
`SessionStateBuilder::with_physical_optimizer_rule` appends after
`SanityCheckPlan`. A rule that breaks a requirement then produces wrong results
instead of an error. On `main`, appending a rule that removes `SortExec`s:
```rust
let state = SessionStateBuilder::new()
.with_default_features()
.with_physical_optimizer_rule(Arc::new(RemoveSorts))
.build();
let ctx = SessionContext::new_with_state(state);
ctx.sql("CREATE TABLE t(a INT) AS VALUES (3), (1),
(2)").await?.collect().await?;
ctx.sql("SELECT a, row_number() OVER (ORDER BY a) AS rn FROM
t").await?.show().await?;
```
```text
+---+----+
| a | rn |
+---+----+
| 3 | 1 |
| 1 | 2 |
| 2 | 3 |
+---+----+
```
The window runs over unsorted input, and the plan is never checked.
2. **Custom pipelines keep the rule last by hand.** For example, GreptimeDB
removes `SanityCheckPlan` and pushes it again after its own rules, and Spice
inserts its rules relative to its position. A custom rule list that leaves it
out runs plans that do not satisfy their requirements.
3. **The error does not say which rule broke the plan.** It only says
`SanityCheckPlan`, so each report needed a manual bisect over the rules. #26391
(`ProjectionPushdown`), #17077 and #25921 are recent examples.
The physical planner already checks
`check_invariants(InvariantLevel::Executable)` after the last rule, for every
pipeline. `SanityCheckPlan` repeats that walk.
### Describe the solution you'd like
Run the `SanityCheckPlan` check in the physical planner after the last rule,
in place of the existing `InvariantChecker(Executable)` walk, and drop the rule
from the default list. When ordering or distribution requirements fail, name
the rule after which they stopped holding. Only plans from the last
`EnsureRequirements` run onwards need to satisfy them, so only those are
searched.
This makes the validity half of option 3 in #26171 checked for every
pipeline: a custom pipeline must keep the plan valid, and if it does not, it
fails at planning time and names the rule. It also gives the
alternative-pipeline sqllogictest mode proposed there a check that does not
depend on where `SanityCheckPlan` sits in the list.
### Describe alternatives you've considered
- Insert appended rules before `SanityCheckPlan`. This fixes 1 for appended
rules only, not for custom rule lists or the error message.
- An invariant region API (#26360). This PR follows the review there: one
rule (`EnsureRequirements`) makes the plan valid, every later rule keeps it
valid, and that is always checked.
- A config option to skip the check. A plan that fails it can return wrong
results, so I would rather not add one.
### Additional context
Related: #26171, #26351, #26360, #25688.
--
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]
