andygrove opened a new pull request, #5220: URL: https://github.com/apache/datafusion-comet/pull/5220
## Which issue does this PR close? Addresses item 6 of #5199. ## Rationale for this change `PlanDataInjector.injectPlanData` runs 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. Every `Filter`, `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. `injectPlanData` now: - returns the node unchanged when neither it nor any descendant was injected (identity-compared per child), and - rebuilds a node only when one of its children actually changed. So 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. Local measurement of `injectPlanData` alone (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): | plan shape | old | new | speedup | | --- | --- | --- | --- | | 20 ops, 1 col | 3.62 us | 2.85 us | 1.27x | | 60 ops, 1 col | 8.81 us | 6.31 us | 1.40x | | 60 ops, 20 cols | 24.72 us | 21.72 us | 1.14x | | 20 ops, 50 cols | 45.21 us | 44.14 us | 1.02x | | 5 ops, 200 cols | 175.54 us | 172.91 us | 1.02x | 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. ## How are these changes tested? - `PlanDataInjectorSuite` — extended the existing "non-scan operator tree unchanged" test to assert reference identity, and added a test that injects into a scan nested under a filter beside an untouched sibling subtree, asserting the sibling is shared, the path to the scan is rebuilt, the injected data is correct, and surrounding fields are preserved. - Existing suites pass: `PlanDataInjectorSuite`, `CometScanWithPlanDataSuite`, `CometExecSuite` (143 tests), `CometJoinSuite`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
