feat: make StatisticsRegistry the default operator-statistics path - #23651
feat: make StatisticsRegistry the default operator-statistics path#23651asolimando wants to merge 16 commits into
Conversation
github-actions
Bot
added
documentation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23651 +/- ##
==========================================
+ Coverage 80.69% 80.85% +0.15%
==========================================
Files 1095 1098 +3
Lines 372626 374693 +2067
Branches 372626 374693 +2067
==========================================
+ Hits 300707 302956 +2249
+ Misses 53968 53679 -289
- Partials 17951 18058 +107 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1f2d39d to
38d5bb1
Compare
StatisticsContext (the single statistics walk, apache#23051) now consults the StatisticsRegistry (apache#21483) at each node before falling back to the operator's statistics_from_inputs, using the provider result's base Statistics. The registry is always-on: datafusion.optimizer.use_statistics_registry is deprecated and ignored (register providers on the SessionState instead). The now-redundant DefaultStatisticsProvider is deprecated, since the walk falls back natively.
The walk keeps core `Statistics` as its currency; a per-walk side cache carries provider `Extensions` separately, so a walk with no providers has no per-node overhead. A provider's `Computed` extensions are cached and reach parent-node providers as their `child_stats`; the built-in `statistics_from_inputs` fallback yields none, so extensions survive only across an unbroken chain of provider-handled nodes. Adds `StatisticsContext::compute_extended` (statistics + extensions) alongside `compute` (core statistics only). `StatisticsRegistry::compute` preserves extensions again; it and `compute_base` are deprecated in favor of `StatisticsContext`.
EXPLAIN ... show_statistics computed statistics with an empty registry, so registered providers were not reflected in the statistics column. Thread the session's StatisticsRegistry into the displayable plan; the column now reflects provider-supplied estimates when providers are registered, and is unchanged when none are.
A runnable example showing a StatisticsProvider that supplies and refines column statistics (post-filter distinct count via the survival formula) to flip a join's build side, with a before/after EXPLAIN.
38d5bb1 to
ded4292
Compare
| STORED AS PARQUET | ||
| LOCATION 'test_files/scratch/statistics_registry/dim_small.parquet'; | ||
|
|
||
| # -- Without registry -------------------------------------------------------- |
There was a problem hiding this comment.
Having removed use_statistics_registry config knob means the registry can't be turned off within an SLT. The no-provider path is covered by existing tests indirectly, and the with/without contrast now lives in the join_reorder example
| } | ||
| struct StatsCache { | ||
| statistics: HashMap<CacheKey, Arc<Statistics>>, | ||
| extensions: HashMap<CacheKey, Extensions>, |
There was a problem hiding this comment.
I kept https://github.com/apache/datafusion/pull/23651/changes#diff-149914d58cc354ddd4d84e283de4f8f07f3295b39137d850b3344448d4f6dc7a separated on purpose in case reviewers prefer to drop it from this PR.
The reason to have two caches is that caching on ExtendedStatistics at every node regressed the compute_statistics benchmark ~6% to 28% (median ~13%) on the no-provider path (a wrapper allocation per node).
The committed version keeps core Statistics in the walk and holds Extensions in a side map, populated only when a provider returns some. Benchmark parity when no providers are registered, extensions still reach parent-node providers.
I think this commit is worth keeping for feature parity w.r.t. StatisticsRegistry as existing today.
| metric_types: self.metric_types, | ||
| metric_categories: self.metric_categories, | ||
| format: self.format, | ||
| statistics_registry: self.statistics_registry, |
There was a problem hiding this comment.
The registry here is needed to keep EXPLAIN ANALYZE and EXPLAIN in sync when statistics providers are in use.
|
As discussed in #21122 (comment), making cc:
Note: the diff reads large (+1055 / -332), but ~620 of the added lines are examples and tests, ~38 are docs. The core change is ~400 lines, which goes down to a more reasonable ~300 if you exclude mechanical fixes. |
There was a problem hiding this comment.
@asolimando
Thanks for working on this.
I left two small suggestions to reduce duplication and make future display-related changes easier to maintain.
|
run benchmark sql_planner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing asolimando/statistics-registry-default (5f2b7f1) to 7f6cc60 (merge-base) diff using: sql_planner File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagesql_planner — base (merge-base)
sql_planner — branch
File an issue against this benchmark runner |
There was a problem hiding this comment.
@asolimando
Thanks for the iteration. The provider-specific child statistics work is heading in the right direction, but there is still one behavior issue that should be addressed before merging. I also noticed a small documentation mismatch.
| let children = plan.children(); | ||
| let requests = plan.child_stats_requests(partition); | ||
| // Resolved for the built-in `statistics_from_inputs` fallback below. | ||
| let child_statistics = self.resolve_children(plan, &children, &requests)?; |
There was a problem hiding this comment.
Could we try the provider path before resolving the operator's child statistics requests?
Right now, compute_base calls plan.child_stats_requests() before trying any provider. This means an operator's fallback-only child walk still runs even when a matching provider requests ChildStats::Skip and can compute the node's statistics itself. Besides doing unnecessary work, an error from that child walk prevents the provider from overriding the node.
The existing test_provider_opts_out_of_child_stats confirms that the provider receives an unknown placeholder, but it does not confirm that the child computation was actually skipped.
Please resolve each provider's own requests while trying that provider, then resolve the operator's requests only after every provider delegates, immediately before calling statistics_from_inputs. It would also be helpful to add a regression test where the operator requests At(...), the matching provider requests Skip, and the test asserts that the child is never computed.
There was a problem hiding this comment.
It makes sense, it's true that children statistics computation can fail, and that would be unfortunate for providers that don't even need those stats, so it makes sense to test the providers first, I have implemented this in 0b6a021.
I have added also a test to cover against regressions in this area (child throws error for statistics, but the provider Skips statistics, so it succeeds with the updated code).
| /// | ||
| /// If no providers are registered, falls back to the plan's built-in | ||
| /// `partition_statistics(None)` with no overhead. | ||
| /// Children are resolved via [`ExecutionPlan::child_stats_requests`] (default |
There was a problem hiding this comment.
Could we update or remove this paragraph? It still says child statistics are resolved through ExecutionPlan::child_stats_requests and that the operator must declare At.
The follow-up moved that responsibility to StatisticsProvider::child_stats_requests, which allows providers for existing operators to request child statistics without changing those operators. Since this deprecated wrapper now follows the provider-specific behavior, the rustdoc should describe that behavior instead.
There was a problem hiding this comment.
Thanks for pointing that out, I have updated (in 0b6a021) the comment to reflect the newly introduced StatisticsProvider::child_stats_requests.
26095c7 to
b2548db
Compare
There was a problem hiding this comment.
@asolimando
Thanks for iteration.
I found one remaining issue around provider delegation. A provider that does not handle the current node can still trigger child statistics computation before a later provider or the operator fallback gets a chance to run. This can introduce unexpected work or surface child errors that should have been avoided.
| let partition = args.partition(); | ||
| for provider in providers { | ||
| let requests = provider.child_stats_requests(plan, partition); | ||
| let child_statistics = self.resolve_children(plan, children, &requests)?; |
There was a problem hiding this comment.
Could we defer resolving a provider's child_stats_requests until we know that the provider will handle the current node?
Right now, try_provider_stats resolves each provider's child statistics request before calling that provider. As a result, a provider that eventually returns Delegate can still force child statistics to be computed. If that child walk fails, the next provider or the operator fallback never gets a chance to run.
The current regression test covers one matching provider that requests ChildStats::Skip, but it does not cover a provider chain. For example, provider A could use the default child_stats_requests value of At(None) and then delegate, while provider B would handle the node using Skip. Provider A's child walk can fail before provider B is called. The same issue affects an operator fallback whose ExecutionPlan::child_stats_requests is Skip, since registering an unrelated provider can unexpectedly cause all child statistics to be computed.
An applicability hook such as supports(plan) could be checked before resolving child statistics. An equivalent lazy design would also work, as long as child statistics are only resolved when the provider actually computes the result.
Please also add a regression test with a delegating provider before a Skip provider and an erroring child, then assert that the later provider still succeeds.
There was a problem hiding this comment.
Thanks for the suggestion, I have implemented it and it doesn't only make performance better, it also allowed to move those "guards" into a dedicated place, while before they had to live in compute, now there is a better separation of concerns and better overall readability!
I have added in 83b5e8f2c a matches(&self, plan) -> bool guard to StatisticsProvider (default to true), which is checked in try_provider_stats, only providers returning true are invoked and they don't resolve children statistics unnecessarily. There was a name clash on supports so I went for matches, we can revisit this if needed.
I have taken advantage of this new capability and I have migrated existing providers, tests and examples (code looks cleaner), and I have added test_non_matching_provider_does_not_trigger_child_walk to cover providers, which is analogous to the previous test_provider_opts_out_of_child_stats, which was covering providers vs built-in.
…cs-registry-default # Conflicts: # datafusion/physical-plan/src/display.rs
There was a problem hiding this comment.
@asolimando
Thanks for continuing to refine the provider statistics flow. The explicit matches() hook improves the new provider path, but there is still a compatibility issue for existing providers that rely on returning Delegate from compute_statistics. I think this needs one more change.
| } | ||
| let partition = args.partition(); | ||
| for provider in providers { | ||
| if !provider.matches(plan) { |
There was a problem hiding this comment.
Could we preserve the existing Delegate contract without requiring every provider to implement matches()?
The new matches() hook avoids child-stat resolution when a provider explicitly returns false. However, its default is true, so existing providers that inspect plan inside compute_statistics and return Delegate for unsupported nodes still resolve their child-stat requests first.
That means an unrelated provider can trigger the default At(None) child walk and fail before a later provider that requests ChildStats::Skip, or before the operator fallback, gets a chance to handle the node. ClosureStatisticsProvider::new appears to have the same issue because it uses an always-true matcher.
A lazy child-stat design may help here, so child statistics are only computed once the provider actually needs them. Please also add a regression test where a provider that does not override matches() returns Delegate, comes before a child-skipping provider, and has an erroring child. The later provider should still succeed.
There was a problem hiding this comment.
Thanks @kosiew, your review comments are really helpful.
Re. the lazy vs eager design: I have "inherited" the eager child statistics resolution from #20184 and #22958 (implemented in #23051): the external StatisticsContext resolves child statistics first, then hands them to statistics_from_inputs (and here, to providers), so a node expresses only local propagation logic and externally-computed statistics (external catalogs, past-run feedback, ML cardinality estimators) can be injected through the same path.
I have considered a lazy design as an alternative but I didn't pursue it since the eager design was already backed by multiple maintainers in the issues/PRs linked above, after extensive discussion, so I considered the decision hard to reconsider.
The fatal-error problem you raise is important, but there is a middle ground solution to handle that in the eager design: if a provider's child walk fails, skip that provider and let a later one or the operator fallback handle the node.
Concretely, it would read as something like this:
let child_statistics = match self.resolve_children(plan, children, &requests) {
Ok(child_statistics) => child_statistics,
Err(e) => {
debug!("Statistics provider {provider:?} skipped for {}: ... {e}", ...);
continue;
}
};Note that this isn't error-swallowing: if the child is genuinely needed even by the built-in fallback, the error resurfaces there. A matched provider's own compute error stays fatal (which seems fair, and matches similar patterns like physical rules). Today each provider is responsible for its own recoverable errors: it can return Delegate for transient failures (e.g., network calls to an external catalog) and surface Err only for hard cases. If you prefer, we could instead catch compute errors the same way as the child walk.
I have optimistically implemented this solution in f9355ca to ground the discussion (along with the regression test you asked for), happy to take a different route if you prefer.
I think this keeps the good sides of both designs: robust over errors, doesn't go against previously agreed eager evaluation for statistics.
In terms of performance I think we are safe too, there is enough machinery to prevent waste for providers:
matches()can easily express operator-level applicability which is probably the vast majority of casesDelegateincompute()for finer-grained checks (e.g., I handleProjectionExecbut only a subset of expressions, and delegate on the rest)ChildStats::Skipfor avoiding computing unneeded stats for some children- caching via
StatisticsContextso repeated stats computation across providers/built-in doesn't trigger multiple walks
What do you think?
There was a problem hiding this comment.
Thanks, this explanation is very helpful and I agree with keeping the eager model for now.
I also agree with your middle-ground goal (do not let an early delegating provider block later providers or fallback), and the new regression test for that scenario is great.
I still see one remaining contract risk in the current implementation: all provider child-resolution errors are treated as skippable. That can mask provider bugs or contract violations (for example, malformed child_stats_requests) and silently disable provider logic.
Could we narrow the skip behavior to only the delegation-related failure mode, while still surfacing hard/provider-contract failures? In other words:
- keep the resilience benefit for the delegate-chain case
- preserve fail-fast behavior for invalid provider requests and other hard errors
There was a problem hiding this comment.
@asolimando
Thanks for the follow-up. I think the overall direction looks good and the rationale for preserving eager child resolution makes sense. The EXPLAIN/ANALYZE registry propagation, display deduplication, constructor cleanup, contract restoration, and the new regression test all address the earlier feedback.
I do think there is one remaining issue before this is ready. The current child resolution error handling is now too broad and can end up masking provider bugs by treating every failure as skippable.
| let partition = args.partition(); | ||
| for provider in providers { | ||
| if !provider.matches(plan) { | ||
| continue; |
There was a problem hiding this comment.
I like the delegate-chain fix, but I think we're now skipping too much.
At the moment, every resolve_children(...) error falls into the same continue path. That means provider contract violations and other hard failures are treated the same as delegation failures, which can silently disable provider behavior and make bugs much harder to spot.
Could we narrow the classification here so that only delegation-related failures are skipped? I'd keep the current eager resolution model, but continue to fail fast for provider contract violations, especially request-length mismatches, and other hard errors.
There was a problem hiding this comment.
Thanks @kosiew, I agree that for invalid provider requests (I added out-of-bounds partitions to the malformed request length you mentioned) it’s good to fail fast.
To that effect, in 08845ef I've split validation from resolution: validating the provider's requests now fails fast, while a child-statistics computation error during resolution is passed on to the next provider (or the operator fallback).
I have added two regression tests to verify that both a wrong-length request set and an out-of-range partition request fail fast and don’t go to the next provider.
6 participants
Which issue does this PR close?
Rationale for this change
#23051 unified statistics into one
StatisticsContextwalk. This folds thepreviously opt-in
StatisticsRegistry(#21483) into that walk as the default, soproviders are consulted inline and
use_statistics_registryis no longer needed.Consumed today by join ordering and EXPLAIN; more CBO rules are follow-up.
What changes are included in this PR?
StatisticsContextwalk (firstComputedwins, else
statistics_from_inputs); used byjoin_selection.use_statistics_registry(ignored, warns) andDefaultStatisticsProvider; add partition-awareStatisticsProvider::compute_statistics_with_args.Extensionsthrough the walk via a side cache (no costwithout providers); add
StatisticsContext::compute_extended, deprecateStatisticsRegistry::compute/compute_base.show_statisticsnow reflect the session registry.join_reorderexample.Are these changes tested?
Unit tests,
statistics_registry.slt, the example, the full extended suite, andcompute_statisticsat benchmark parity.Are there any user-facing changes?
use_statistics_registrydeprecated and ignored (default behavior unchanged).StatisticsRegistry::compute/compute_basedeprecated (55.0.0) forStatisticsContext;compute_extendedadded;DefaultStatisticsProviderdeprecated. Nothing removed. See the 55.0.0 upgrade guide.
Disclaimer: I used AI to assist in the code generation, I have manually reviewed the output and it matches my intention and understanding.