Skip to content

fix: implement replace_children on distributed plan nodes - #668

Closed
shinzoxD wants to merge 2 commits into
datafusion-contrib:mainfrom
shinzoxD:fix/replace-children-657
Closed

fix: implement replace_children on distributed plan nodes#668
shinzoxD wants to merge 2 commits into
datafusion-contrib:mainfrom
shinzoxD:fix/replace-children-657

Conversation

@shinzoxD

@shinzoxD shinzoxD commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Closes #657

ExecutionPlan::replace_children is a DataFusion 55 API. branch-55 was merged to main in #540 (v4.0.0), so this targets main.

with_new_children is deprecated in favor of replace_children. This implements replace_children on our custom plan nodes. Existing with_new_children callers keep working; they forward ChildrenPropertiesMode::Recompute.

Types:

  • BroadcastExec, SamplerExec, DistributedLeafExec, MetricsWrapperExec, ChildrenIsolatorUnionExec
  • NetworkBroadcastExec, NetworkCoalesceExec, NetworkShuffleExec
  • DistributedExec, DistributedAnalyzeExec
  • test helpers: MockExec, URLEmitterExec, RowGeneratorExec

Replaces closed draft #658.

@shinzoxD

Copy link
Copy Markdown
Contributor Author

CI fix pushed: 4203aa15fae22f61edab70cc68ddfa26db04a432

clippy: needless_borrow in src/execution_plans/broadcast.rs replace_children_swaps_input. Arc::ptr_eq already takes &Arc<T>, and replaced.children()[0] is &Arc<_>, so the extra & was immediately dereferenced. Changed to Arc::ptr_eq(replaced.children()[0], &replacement) (and the same for original).

unit-test tests::build_side_fetch_is_preserved_by_broadcast: not caused by replace_children.

  • Main CI after fix: look through ProjectionExec when pushing fetch #661 (run 32567634745) passed this test. This PR is that commit plus the with_new_childrenreplace_children rename.
  • Failure was 3750 (distributed) vs 2500 (single-node). 3750 = 75 ids × 50 probe dups: Stage 1 scanned part-1/2/3 (ids 25–99), not part-0. 2500 = 50 × 50 is LIMIT 50 honored. The sibling comment on build_side_fetch_is_preserved_by_normalize documents the same 2500-vs-3750 pair as fetch present vs dropped.
  • The printed plan still has CoalescePartitionsExec: fetch=50 above NetworkBroadcastExec, so insert_broadcast_execs kept fetch. Stage 1 ran with 4 tasks; each join task coalesces a select_all of 4 producer streams. That is the known LIMIT-without-ORDER-BY + multi-task broadcast shape, not a dropped fetch field.
  • BroadcastExec::replace_children is the previous with_new_children body (Self::new(child, consumer_task_count)). DF 55’s default replace_children already forwarded Recompute there. No production-path change on the broadcast/fetch pipeline.
  • Sibling build_side_fetch_is_preserved_by_normalize passed in the same failing run.

Left the test alone: fixing the 4-task LIMIT shape is #624, which this PR should not mix in. If unit-test is red again on this SHA, treat it as that pre-existing flake rather than a replace_children regression.

Did not undraft or merge.

@gabotechs gabotechs closed this Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

implement replace_children for all ExecutinoPlan implementations

2 participants