zhuqi-lucas opened a new issue, #25355:
URL: https://github.com/apache/datafusion/issues/25355
### Is your feature request related to a problem or challenge?
DataFusion's two optimizers take opposite positions on repeated rule
execution, and the physical side is the one without a mechanism.
The **logical** optimizer iterates (`max_passes`, default 3) and detects
convergence: it records a `LogicalPlanSignature` (node count + plan hash) per
pass and stops once a plan repeats (`datafusion/optimizer/src/optimizer.rs`).
The **physical** optimizer runs its rule list exactly once, in a
hand-ordered sequence, with no iteration, no convergence detection, and no way
for a rule to recognize that its input is the very plan it produced a moment
ago. That works for the default chain, which is carefully ordered so that
everything able to invalidate distribution/ordering requirements runs *before*
the single `EnsureRequirements` — the per-rule comments in
`physical-optimizer/src/optimizer.rs` say so explicitly.
It stops working as soon as a chain is assembled with rules *after* that
point. Downstream projects do this routinely: DataFusion is a library,
`with_physical_optimizer_rules` is a supported extension point, and a rule
inserted late (a custom scan rewrite, a distributed-execution boundary, an MV
substitution) invalidates requirements again and needs another enforcement pass
behind it. Each of those passes is real work — a full plan traversal
recomputing requirements — and some of them run on a plan that no preceding
rule touched.
This is not a DataFusion-specific shape. DuckDB's pipeline runs
`CTE_INLINING`, `UNUSED_COLUMNS` and `COLUMN_LIFETIME` twice with no dedup at
all. Spark Catalyst runs the same ~30-rule set twice around "Infer Filters".
The difference is that the others grew a mechanism for it:
| Engine | Mechanism | Granularity |
| --- | --- | --- |
| Spark Catalyst | per-`TreeNode` `_ineffectiveRules` BitSet, plus batch
fixpoint via `fastEquals` | rule × subtree, opt-in at the call site |
| ClickHouse (QueryPlan) | a pass returns `size_t update_depth`, re-descent
bounded by it | pass × node |
| Calcite HepPlanner | `firedRulesCache` keyed on matched node ids, opt-in
via `setEnableFiredRulesCache` (new in 1.42, CALCITE-7416/7422) | rule × match |
| StarRocks / Doris | Cascades `ruleMasks` BitSet | rule × GroupExpression |
| Trino / Presto | explicit `Optional` per invocation, no structural
comparison | rule invocation |
### Describe the solution you'd like
The lightest thing that fits DataFusion's existing semantics. Physical rules
already return the input `Arc` untouched when they change nothing, so pointer
identity is an exact and free "nothing happened" signal — no hashing, no
structural comparison.
1. **A defaulted trait method**, so every existing rule is unaffected:
```rust
/// Whether this rule is a pure function of the plan it is given, so
running
/// it again on a plan object it previously returned cannot change
anything.
///
/// Rules that read state outside the plan (session state that can change
/// between invocations, counters, randomness) must leave this `false`.
fn skip_if_unchanged(&self) -> bool { false }
```
2. **Keep the state in the optimizer loop, not on the rule.**
`optimize_physical_plan` holds a per-run `Vec<Option<*const _>>` of each rule's
last output pointer and skips when the current input matches. Rules stay
stateless, and nothing leaks across queries or between concurrently planned
statements — a real hazard if the memo lived on the rule, since rule instances
are shared.
3. **Verify idempotence in debug builds.** When a skip would fire under
`debug_assertions`, run the rule anyway and assert the result is unchanged.
Spark is the only surveyed engine that tests this
(`RuleExecutor.checkBatchIdempotence` under `Utils.isTesting`), and the engines
that rely on counters or `Optional` instead have public incidents from
non-idempotent rules looping (trinodb/trino#11559, prestodb/presto#9362).
Catching it at test time is cheap.
4. **Gate it behind a config flag**, default off, e.g.
`datafusion.optimizer.skip_unchanged_physical_rules` — mirroring how Calcite
shipped the same capability opt-in.
The default chain would see little or no change, since it has no repeated
rule aside from `ProjectionPushdown` (and seven rules separated by, so the
pointer rarely matches). The benefit lands on custom chains, which is precisely
where the framework currently offers nothing.
### Describe alternatives you've considered
- **Structural signatures, like the logical optimizer's
`LogicalPlanSignature`.** More general (it catches "different object, same
plan"), but it costs a hash of the whole plan per rule, which on a large plan
is comparable to just running a cheap rule. Pointer identity is exact for the
case that matters and costs nothing.
- **ClickHouse's `update_depth`.** Strictly more information than a boolean
and it bounds re-traversal precisely, but it changes the signature of every
rule — too invasive for the value here.
- **Leaving it to downstream wrappers.** That is what happens today, and it
has its own trap: a wrapper rule must also forward `schema_check()` of the
rules it wraps, or their validation silently disappears (#25316).
### Additional context
Happy to implement this if the direction is welcome.
--
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]
