feat: make StatisticsRegistry the default operator-statistics path - #23651
Open
asolimando wants to merge 16 commits into
Open
feat: make StatisticsRegistry the default operator-statistics path#23651asolimando wants to merge 16 commits into
asolimando wants to merge 16 commits into
Conversation
|
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:
|
asolimando
force-pushed
the
asolimando/statistics-registry-default
branch
from
July 16, 2026 20:19
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.
asolimando
force-pushed
the
asolimando/statistics-registry-default
branch
from
July 17, 2026 18:38
38d5bb1 to
ded4292
Compare
asolimando
commented
Jul 17, 2026
| STORED AS PARQUET | ||
| LOCATION 'test_files/scratch/statistics_registry/dim_small.parquet'; | ||
|
|
||
| # -- Without registry -------------------------------------------------------- |
Member
Author
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
asolimando
commented
Jul 17, 2026
| } | ||
| struct StatsCache { | ||
| statistics: HashMap<CacheKey, Arc<Statistics>>, | ||
| extensions: HashMap<CacheKey, Extensions>, |
Member
Author
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.
asolimando
commented
Jul 17, 2026
| metric_types: self.metric_types, | ||
| metric_categories: self.metric_categories, | ||
| format: self.format, | ||
| statistics_registry: self.statistics_registry, |
Member
Author
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.
asolimando
marked this pull request as ready for review
July 17, 2026 19:30
Member
Author
|
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. |
kosiew
reviewed
Jul 20, 2026
Contributor
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.
xudong963
requested changes
Jul 22, 2026
Contributor
|
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 |
kosiew
reviewed
Jul 23, 2026
Contributor
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.
asolimando
force-pushed
the
asolimando/statistics-registry-default
branch
from
July 27, 2026 16:18
26095c7 to
b2548db
Compare
kosiew
requested changes
Jul 28, 2026
Contributor
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.
…cs-registry-default # Conflicts: # datafusion/physical-plan/src/display.rs
kosiew
requested changes
Jul 30, 2026
Contributor
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.
kosiew
requested changes
Jul 30, 2026
Contributor
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.
kosiew
approved these changes
Jul 31, 2026
Member
Author
|
Hey @xudong963, you seem happy with the modifications you required earlier, but I still see the "request changes" status. Is there anything missing you have already identified, or do you just want to take another look? |
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
6 participants
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?
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.