asolimando opened a new pull request, #26094: URL: https://github.com/apache/datafusion/pull/26094
## Which issue does this PR close? - Part of #8227. - Related to #25141. ## Rationale for this change Each physical optimizer rule that reads statistics creates its own `StatisticsContext`, so the statistics of the same plan nodes are computed again in every rule. `JoinSelection` creates a new context for every `get_stats` call, so it has no caching at all, even within its own pass. #25098 showed the gain of sharing one context inside `EnsureDistribution`. #25929 made cache entries keep the plan node they were computed for, so a context can now be shared safely across plan rewrites. This PR uses that to share one `StatisticsContext` across all the rules of one `optimize_physical_plan` call. On TPC-DS (98 queries) and TPC-H (21 queries), sf1 Parquet, planned with a local harness (not part of this PR): | | `main` | this PR | |---|---|---| | Statistics cache misses (statistics computed from scratch) | 46,022 | 25,428 (-45%) | | Physical planning time, sum over all queries | 294.8 / 293.5 ms | 274.3 / 279.7 ms (about -6%) | | Physical plans | | identical for all 119 queries | The largest gain is TPC-DS q64: cache misses go from 3,785 to 531 and planning time drops by about 21%. ## What changes are included in this PR? - `StatisticsContext` stores its cache in a `parking_lot::Mutex` instead of `Rc<RefCell>`, so it is `Send + Sync`. This is needed because `PhysicalOptimizerContext: Send + Sync`. The lock is held only for single map lookups and inserts, never across the recursive walk. The `compute_statistics` benchmark shows no difference from `main` (all cases within ±4%, in both directions). - New `PhysicalOptimizerContext::statistics_context()`, which returns `None` by default. `DefaultPhysicalPlanner` creates one context per `optimize_physical_plan` call, built from the session's statistics registry, and returns it to every rule. - `ConfigOnlyContext` also owns a `StatisticsContext`, so a rule called through `optimize()` shares one cache for its whole pass. - `JoinSelection`, `EnsureRequirements` (including `PlanSize::from_plan`), `AggregateStatistics` and `LimitPushdown` use the shared context. When a context does not share one (for example, a context received through FFI), the rules create a new context from the statistics registry, as they did before. - `pushdown_limit_helper` keeps its signature and calls the new `pushdown_limit_helper_with_stats`, which takes a `&StatisticsContext`. This is the same pattern as `ensure_distribution_with_stats`. ## What is the testing strategy for this PR? - New `optimizer_rules_share_statistics_context` test in `physical_planner.rs`: two rules compute the root statistics through the shared context, and the second one gets the `Arc` cached by the first. - Existing tests cover the rewired rules. All sqllogictests pass with no plan changes. ## Are there any user-facing changes? No breaking changes. - New default method `PhysicalOptimizerContext::statistics_context()`. - New public function `pushdown_limit_helper_with_stats`. - `StatisticsContext` is now `Send + Sync`. - `AggregateStatistics`, `LimitPushdown` and `PlanSize::from_plan` now consult the session's statistics providers, as `JoinSelection` and `EnsureRequirements` already do. Without registered providers (the default), their results are unchanged. ---- Disclaimer: I used AI to assist in the code generation, I have manually reviewed the output and it matches my intention and understanding. -- 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]
