Repository navigation
fix: use session statistics registry in dfbench reports - #25570
Merged
gabotechs merged 4 commits intoOct 6, 2026
Merged
Conversation
gabotechs
marked this pull request as ready for review
September 21, 2026 15:32
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25570 +/- ##
==========================================
- Coverage 82.70% 82.70% -0.01%
==========================================
Files 1147 1147
Lines 447631 447668 +37
Branches 447631 447668 +37
==========================================
+ Hits 370222 370250 +28
- Misses 54921 54927 +6
- Partials 22488 22491 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This was referenced Sep 22, 2026
Contributor
Author
|
cc @kosiew, as you gave a great review on the original PR. It's a trival change that should take no more than 2 mins to review. |
Member
kosiew
requested changes
Oct 5, 2026
kosiew
left a comment
kosiew
left a comment
Contributor
There was a problem hiding this comment.
Thanks for working on this. The implementation addresses the registry mismatch, but the behavior change itself is not covered by a regression test yet.
kosiew
approved these changes
Oct 6, 2026
kosiew
left a comment
kosiew
left a comment
Contributor
There was a problem hiding this comment.
Thanks for adding the regression test. It now verifies the user-visible behavior by using a distinguishing session statistics provider and asserting that dfbench reports that estimate.
Contributor
Author
|
Thanks @kosiew and @asolimando! |
gabotechs
added a commit
to gabotechs/datafusion
that referenced
this pull request
Oct 6, 2026
## Which issue does this PR close? - Closes apache#25571. ## Rationale for this change `StatisticsRegistry::default_with_builtin_providers()` registers providers that are not the default estimation and that duplicate or replace what the operators already do in `statistics_from_inputs`. They can make estimates worse (TPC-H Q14: 14.73 billion rows instead of 73,650, see apache#25570), and fixes in the operators do not reach their users. Estimation improvements belong in the operators; `StatisticsRegistry` stays as the extension point for user-defined providers. ## What changes are included in this PR? - `default_with_builtin_providers()` and all seven bundled providers are deprecated. Their code is unchanged. - `statistics_registry.slt` and the `join_reorder` example use user-defined providers instead of bundled ones. - `dfbench statistics` reports the operators' own estimates instead of the bundled providers' estimates. Follow-ups: moving the Filter distinct count survival model into `FilterExec` (apache#26052); multi-key join estimation is tracked in apache#21583. ## What is the testing strategy for this PR? `statistics_registry.slt` passes unchanged; the `join_reorder` example still flips the build side. ## Are there any user-facing changes? Deprecations only, see the 56.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. --------- Co-authored-by: Gabriel <45515538+gabotechs@users.noreply.github.com>
gabotechs
enabled auto-merge
October 6, 2026 08:42
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
4 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?
Part of #8227.
Rationale for this change
dfbench statisticsunconditionally enables built-in statistics providers, so its estimates can differ from those used by the session. In TPC-H SF1 Q14, it reports 14.73 billion rows instead of 73,650 for a join that produces 75,983 rows.Unrelated to this PR, but it's pretty weird how the built-in providers are not actually built-in unless explicitly enabled, and it's also weird how they can override the actually built-in code in each operator producing different results. It seems like there's different pieces of code with overlapping intentions and different implementation.
What changes are included in this PR?
Capture estimates using the session's registry, matching
EXPLAIN ANALYZE. An unconfigured session falls back to operator statistics.What is the testing strategy for this PR?
Are there any user-facing changes?
CLI estimates now respect the session's statistics configuration.