kosiew commented on code in PR #25570:
URL: https://github.com/apache/datafusion/pull/25570#discussion_r4182877552
##########
benchmarks/src/statistics.rs:
##########
@@ -184,7 +184,10 @@ impl RunOpt {
let logical_plan = state.optimize(&logical_plan)?;
let physical_plan = state.create_physical_plan(&logical_plan).await?;
- let statistics = capture_statistics(physical_plan.as_ref())?;
+ let statistics = capture_statistics(
Review Comment:
In other words - Does the lack of production-user exposure make a regression
test for the statistics registry unnecessary?
Respectfully, no. The test need not name or pin
`default_with_builtin_providers()`. The contract change is observable through
dfbench: reported estimates must come from the same `SessionState` statistics
registry that planned the query. The current tests only check whether reports
are empty, so reverting the change to
`StatisticsRegistry::default_with_builtin_providers()` still passes.
Add a `report_statement` test with a `SessionStateBuilder` registry
containing a distinguishing `ClosureStatisticsProvider` (for example,
`Inexact(42)` rows), then assert that estimate in the returned report. That
provider is controlled input, not an assertion about registry composition. It
fails before this commit and remains valid through implementation changes
preserving the contract.
--
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]
