zhuqi-lucas opened a new pull request, #24445: URL: https://github.com/apache/datafusion/pull/24445
## Which issue does this close? None yet — happy to file one if that is preferred. ## Rationale for this change `replace_children_if_necessary` already short-circuits two cases: identical child pointers, and identical child `PlanProperties` pointers. A rule that inserts a sort below a projection satisfies neither, because the child is a new object and so its properties pointer differs. Its *equivalence group*, however, is unchanged: sorting changes which orderings hold, not which expressions are equal to one another. `Recompute` therefore re-projects a group identical to the one the projection already holds. `EquivalenceGroup::project` is a pure function of the group and the mapping, so when both are unchanged the previous result can be handed back instead of recomputed. This matters on plans with many wide projections, because the recompute is repeated for every projection above the inserted sort, on every pass of the rule. ## What changes are included in this PR? 1. `EquivalenceGroup` gains `PartialEq`, comparing `classes`. `map` is an index into `classes` and carries no additional information. 2. `EquivalenceProperties::project` splits, so `project_with_eq_group` can take an already-projected group. Orderings are still derived, since they are precisely what changes when a sort appears below. 3. `ProjectionExec::replace_children` compares the old and new child equivalence groups on the `Recompute` path and, when they match, reuses the cached group. The check is local to `ProjectionExec`. No new `ChildrenPropertiesMode` variant and no change to the shared path, so other operators are unaffected. ## Measurements A query over a 1191-line view with 38 SELECTs, 117 CASE expressions and 7 joins across 11 tables. Ten warm samples per configuration, identical build flags, the only variable being this patch: | | before (median) | after (median) | | | --- | --- | --- | --- | | EnforceSorting | 197.8ms | 90.1ms | −54% | | optimizer rules | 338.1ms | 214.6ms | −37% | | planning wall | 420.6ms | 299.0ms | −29% | The ranges do not overlap: 196.9–199.2 against 88.8–91.0. Logical rules are unchanged across the two configurations, which acts as the control: the saving lands in the physical phase and nowhere else. `EnforceDistribution` improves as well, since it rebuilds the same projections. The saving scales with projection count times expression size times rule passes, so plans with narrow projections should see little. I expect `sql_planner` to show a small delta for that reason. ## Are there any user-facing changes? No. Plans are unchanged. ## Testing `datafusion-physical-expr` 1594, `datafusion-physical-plan` 1716, `datafusion-physical-optimizer` 33, `datafusion-optimizer` 760, `datafusion-common` 550, `datafusion` core 442, and all 502 sqllogictest files. No test or expected plan was modified. Worth flagging for review: an earlier revision passed the *projected* equivalence properties to `Partitioning::project` where `compute_properties` passes the input's. Every unit test stayed green; only `range_partitioning.slt` caught it, via the case asserting that a join preserving Range partitioning lets the aggregate above it skip a Hash repartition. It produced a worse plan rather than a wrong answer, which is why nothing else noticed. ## Follow-up Holding the equivalence group behind an `Arc` would reduce the comparison to `Arc::ptr_eq` and make `EquivalenceProperties::clone` cheaper, which happens on every `SortExec` construction. It touches every site that mutates the group, so it seemed better kept separate. The structural comparison's cost is already inside the numbers above. -- 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]
