zhuqi-lucas opened a new pull request, #25356: URL: https://github.com/apache/datafusion/pull/25356
## Which issue does this PR close? Closes #25355. ## Rationale for this change The logical optimizer iterates and stops on convergence (`max_passes` + `LogicalPlanSignature`). The physical optimizer runs its list once, which suits the default chain: every rule able to invalidate distribution or ordering requirements is deliberately ordered *before* the single `EnsureRequirements`, as the per-rule comments in `physical-optimizer/src/optimizer.rs` state. Custom rule lists do not get that for free. A rewrite inserted after that point — a scan rewrite, a distributed-execution boundary, a view substitution — invalidates requirements again and needs its own enforcement pass behind it. Some of those passes then run on a plan that no preceding rule touched, and each is a full traversal recomputing requirements. Physical rules already return their input `Arc` untouched when they have nothing to do, so pointer identity is an exact and allocation-free "nothing happened" signal — no hashing, no structural comparison. ## What changes are included in this PR? - `PhysicalOptimizerRule::skip_if_unchanged()`, defaulted to `false`, for a rule to declare it is a pure function of the plan it is given. - `datafusion.optimizer.skip_unchanged_physical_rules`, defaulted to `false`. - When both agree, `optimize_physical_plan` remembers the plan each opted-in rule returned and skips the call when handed back that exact object. The memo is scoped to the optimization run and keyed by rule name: rule instances are shared between queries, so this must not live on the rule, and a rule listed twice is normally two instances rather than one. - Debug builds run a skipped rule anyway and assert the result is unchanged, so a rule that declares purity without having it fails a test rather than a query. (Spark is the only engine I found that checks this — `RuleExecutor.checkBatchIdempotence` under `Utils.isTesting`; the engines relying on counters or an `Optional` instead have public incidents from non-idempotent rules, e.g. trinodb/trino#11559.) - Three tests: the skip fires for a repeated rule, it stays inert unless both the rule and the config opt in, and the memo does not leak between plans. Worth knowing for review: because of the debug self-check, the skip saves nothing in debug builds — it saves the pass in release. The tests encode that explicitly rather than hiding it. ## Are these changes tested? Yes — the three tests above, plus the existing suites (`datafusion` lib 448, `datafusion-physical-optimizer` 37) pass in both debug and release. ## Are there any user-facing changes? One new config option and one new trait method, both defaulted off, so existing rules and sessions behave exactly as before. The default rule list has no repeated rules, so it is unaffected either way; the benefit is for custom rule lists assembled via `with_physical_optimizer_rules`. -- 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]
