perf: avoid rebuilding untouched operators in PlanDataInjector.injectPlanData - #5220
Merged
andygrove merged 2 commits intoAug 3, 2026
Merged
Conversation
…PlanData `injectPlanData` runs once per task on the executor and rebuilt every operator in the plan tree through a builder, even though at most one or two leaf scans actually receive injected data. Operators are protobuf messages and therefore immutable, so a subtree that needs no injection can be returned by reference instead. Only the root-to-scan paths are rebuilt now; untouched siblings and their descendants are shared. Addresses item 6 of apache#5199.
…ilder Replaces the per-node child array + changed flag + zero-child special case with a builder created lazily on the first changed child, setting only the changed slots. Unchanged nodes now allocate nothing at all (the previous form allocated a throwaway array per interior node), and the zero-child fast path is no longer needed. Also documents the "injectors must preserve the child list" invariant on `PlanDataInjector.inject`, since contribs implement that trait via ServiceLoader and the walker relies on child reference identity.
mbutrovich
approved these changes
Aug 3, 2026
mbutrovich
left a comment
Contributor
There was a problem hiding this comment.
Makes sense to me, thanks @andygrove!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Addresses item 6 of #5199.
Rationale for this change
PlanDataInjector.injectPlanDataruns once per task on the executor. It recursed into every subtree and rebuilt every operator through a builder (op.toBuilder...clearChildren()...build()), even though at most one or two leaf scans in a plan actually receive injected data. EveryFilter,Projection,HashJoin, etc. above and beside the scans was reconstructed for nothing, once per task.What changes are included in this PR?
Operators are protobuf messages and therefore immutable, so a subtree that needs no injection can be returned by reference rather than rebuilt.
injectPlanDatanow creates a node's builder lazily, on the first child that actually changed, and sets only the changed slots — so a node whose subtree needed no injection is returned as-is and allocates nothing.Only the root-to-scan paths are rebuilt; untouched siblings and their descendants are shared with the input tree. The injection logic, key lookup, and the "missing planning data" error are unchanged.
The
PlanDataInjector.injectscaladoc now states the invariant the walker relies on (implementations must return the node with its child list unchanged), since contribs implement that trait viaServiceLoaderand would otherwise not see the constraint.Local measurement of
injectPlanDataalone (new vs. old implementation, warmed up, interleaved rounds; synthetic plan = N unary operators over a hash join of two native scans with C columns each):The saving is the tree rebuild, so it grows with plan size and shrinks in relative terms as the injection itself (parsing the scan's common bytes, which scales with schema width) comes to dominate. It also removes the corresponding garbage from the per-task path.
Worth noting for the epic: after this change the dominant per-task cost in
CometExecRDD.computeis the fullOperator.parseFrom+ re-serialize that brackets the injection, andNativeScanPlanDataInjector.getKeyrecomputing its string key per scan per task. Both are separate follow-ups.How are these changes tested?
PlanDataInjectorSuite— the existing "non-scan operator tree unchanged" test now asserts reference identity, and a new test injects into a scan nested under a filter beside an untouched sibling subtree, asserting the sibling is shared, the injected data is correct, and surrounding fields are preserved. This suite is already in thepr_build_linux/pr_build_macosmatrices.PlanDataInjectorSuite,CometScanWithPlanDataSuite,CometExecSuite+CometJoinSuite(172 tests).