Skip to content

Enforce with_new_children_if_necessary helper project-wide via clippy lint #22555

Description

@zhuqi-lucas

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. perf(physical-optimizer): skip ensure_distribution rebuild when children are unchanged #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 (Cache PlanProperties, add fast-path for with_new_children #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 Cache PlanProperties, add fast-path for with_new_children #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

Additional context

Related work:

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.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions