Skip to content

[branch-52] perf: skip ensure_distribution rebuild when children are unchanged#57

Merged
zhuqi-lucas merged 1 commit into
branch-52from
qizhu/enforce-dist-skip-rebuild-branch-52
May 29, 2026
Merged

[branch-52] perf: skip ensure_distribution rebuild when children are unchanged#57
zhuqi-lucas merged 1 commit into
branch-52from
qizhu/enforce-dist-skip-rebuild-branch-52

Conversation

@zhuqi-lucas
Copy link
Copy Markdown
Collaborator

Summary

Cherry-pick of upstream apache/datafusion#22521 (approved by 2010YOUY01 and alamb, awaiting maintainer merge) into our branch-52 fork.

ensure_distribution was calling plan.with_new_children(children_plans) unconditionally after collecting the (possibly redistributed) children, even when none of the children were actually replaced. For nodes like ProjectionExec that path recomputes schema / equivalence properties / output ordering / partitioning via try_new — pure overhead when child Arcs are pointer-identical.

Routes the non-UnionExec branch through the existing with_new_children_if_necessary helper, which does the Arc::ptr_eq short-circuit and reuses the input plan when no children changed.

Why pick now

Linear X-2631. Atlas reference-cluster query servers run on branch-52-pinned DataFusion and hit the slow path on every point-query plan node. Profiling on NY5 prod showed ProjectionExec::with_new_children as the single largest cost inside ensure_distribution (1.94s of a 2.87s rule-time slice in a 60s CPU sample).

Adaptations from upstream

  • Upstream lives at datafusion/physical-optimizer/src/ensure_requirements/enforce_distribution.rs; branch-52 keeps the flat path datafusion/physical-optimizer/src/enforce_distribution.rs. Same logic, different module path.
  • Upstream uses plan.is::<UnionExec>(); branch-52 still has plan.as_any().is::<UnionExec>(). Adjusted.

Affects

  • Staging: query servers will see the fast path after a DF rev bump (separate PR).
  • Production: same, after staging soak.

Tests

cargo test --release -p datafusion --test core_integration -- physical_optimizer::enforce_distribution — 63/63 pass (62 pre-existing + 1 new regression test ensure_distribution_reuses_plan_arc_when_no_redistribution_needed).

Micro-benchmark from upstream PR

Plan shape: 30-deep ProjectionExec stack over a sorted parquet scan, 5000 iterations.

  • Without fix: 852.74 ms total, 170.55 us/call
  • With fix: 296.81 ms total, 59.36 us/call
  • ~2.87x speedup, -65% CPU per call

…unchanged

Cherry-pick of upstream apache#22521 (approved by 2010YOUY01
and alamb, awaiting maintainer merge). Adapted to branch-52's file
layout (datafusion/physical-optimizer/src/enforce_distribution.rs vs
the upstream ensure_requirements/ subdirectory) and to the older
plan.as_any().is::<UnionExec>() API.

ensure_distribution was unconditionally calling plan.with_new_children
after collecting the (possibly redistributed) children, even when none
of the children were actually replaced. For nodes like ProjectionExec
that path runs through try_new and recomputes schema, equivalence
properties, output ordering, and partitioning, which is pure overhead
when the input Arcs are pointer-identical.

Routes the non-UnionExec branch through the existing
with_new_children_if_necessary helper, which does the Arc::ptr_eq
short-circuit and reuses the input plan when no children changed.
UnionExec to InterleaveExec special case still runs first because it
intentionally produces a new node even when child Arcs are unchanged.

Linear: X-2631
Copilot AI review requested due to automatic review settings May 28, 2026 08:07
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Cherry-pick of upstream apache#22521 into branch-52 that avoids unnecessary with_new_children rebuilds in ensure_distribution when no child plan was actually replaced, by routing through the existing with_new_children_if_necessary helper (which short-circuits via Arc::ptr_eq). This eliminates redundant schema / equivalence / ordering / partitioning recomputation (notably for ProjectionExec) on plan shapes where no redistribution is needed.

Changes:

  • Switch the non-UnionExec/non-InterleaveExec branch in ensure_distribution from plan.with_new_children(children_plans)? to with_new_children_if_necessary(plan, children_plans)?.
  • Add the with_new_children_if_necessary import from datafusion_physical_plan.
  • Add a regression test verifying that a deep ProjectionExec chain over a single-partition scan returns the same Arc from ensure_distribution.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
datafusion/physical-optimizer/src/enforce_distribution.rs Route the common branch through with_new_children_if_necessary so unchanged children short-circuit the rebuild; add corresponding import and explanatory comment.
datafusion/core/tests/physical_optimizer/enforce_distribution.rs New regression test ensure_distribution_reuses_plan_arc_when_no_redistribution_needed asserting Arc::ptr_eq between input and output for a no-op-distribution projection stack.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@zhuqi-lucas zhuqi-lucas merged commit f62e79d into branch-52 May 29, 2026
59 checks passed
zhuqi-lucas added a commit to massive-com/datafusion-materialized-views that referenced this pull request May 29, 2026
…rebuild) (#47)

Picks up massive-com/arrow-datafusion#57 (cherry-pick of upstream
apache/datafusion#22521): ensure_distribution now routes through
with_new_children_if_necessary, skipping the expensive
plan.with_new_children() rebuild when children are pointer-identical.

Atlas's reference query servers need this DF rev to consume the
optimization end-to-end. MV crate has to bump in lockstep otherwise
atlas ends up with two different DF copies in the workspace
(MV depending on 05a6c45, atlas depending on f62e79d), causing type
mismatches across the MV/atlas boundary.

All 25 + 1 tests pass against the new rev.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants