Enforce with_new_children_if_necessary helper project-wide via clippy lint
- Dominant language
- Rust
- Stars
- 9.3k
- Forks
- 2.4k
- Avg merge
- 3d 7h
- Merged PRs (30d)
- 344
Description
### Is your feature request related to a problem or challenge?
Follow-up suggested by @2010YOUY01 in #22521 and expanded in the comments below.
`datafusion_physical_plan::with_new_children_if_necessary` already exists and is the right way for callers to rebuild a plan from new children — it short-circuits via `Arc::ptr_eq` when children are unchanged, skipping the (often expensive) `with_new_children` recomputation of schema / equivalence properties / output ordering / partitioning.
However:
1. There is **no project-wide enforcement** that callers route through this helper. Optimizer rules and other call sites can — and frequently do — call `plan.with_new_children(children)` directly, missing the optimization. #22521 hit this in `ensure_distribution`; the same pattern likely exists in other optimizer passes.
2. The "skip work when children are unchanged" intent is **split between two layers**: caller-side (this helper, via `Arc::ptr_eq`) and callee-side (#19792, which lives inside individual `ExecutionPlan` impls and avoids `PlanProperties` recomputation). Two layers means two places to maintain and reason about; future changes can drift apart.
### Describe the solution you'd like
Two PRs, in this order so callers that haven't switched yet still benefit from the consolidated behavior:
**PR 1 — Unify with #19792 (callee-side cleanup):**
- Move the `PlanProperties` skip logic introduced in #19792 into `with_new_children_if_necessary`.
- Remove the corresponding logic from the individual `ExecutionPlan` implementations once the helper is the single source of truth.
- After this PR, `with_new_children_if_necessary` is the one place that knows how to skip work when children are unchanged, regardless of who is calling it.
**PR 2 — Enforce caller-side use (clippy lint):**
- Audit existing `plan.with_new_children(children)` call sites across the project. For each, decide whether to migrate to `with_new_children_if_necessary` (typically yes).
- Add a clippy lint (custom lint or `disallowed_methods` config in `clippy.toml`) that forbids direct `ExecutionPlan::with_new_children` outside of the helper itself and a small allow-list of legitimate uses (the trait implementations, tests, and the helper's internal call).
- Document the convention in the contributor guide so new optimizer rules adopt the helper by default.
### Describe alternatives I've considered
- **Leave it as a soft convention** (status quo): relies on reviewer memory; same class of regression keeps appearing whenever someone adds a new optimizer pass.
- **Inline the ptr_eq check at every call site**: tried in the original draft of #22521; rejected by reviewer in favor of the existing helper.
- **Do only the clippy lint, skip the #19792 unification**: still leaves two layers doing the same job. Worth doing the unification while we're here.
### Additional context
Related work:
- #22521 — caller-side fix in `ensure_distribution`, the immediate motivation for filing this issue.
- #19792 — earlier callee-side variant that avoids `PlanProperties` re-computation inside `with_new_children`. To be folded into `with_new_children_if_necessary` per PR 1 above.
This issue tracks the systematic version. Not claiming it for myself — happy to advise but the audit + lint are project-wide scope and someone closer to the day-to-day optimizer pipeline is probably better placed to land it.
Contributor guide
Assessment
This issue has not been assessed yet.