gabotechs commented on code in PR #25969:
URL: https://github.com/apache/datafusion/pull/25969#discussion_r4184513639
##########
datafusion/physical-plan/src/operator_statistics/mod.rs:
##########
@@ -402,6 +397,11 @@ impl StatisticsRegistry {
/// 5. [`JoinStatisticsProvider`]
/// 6. [`LimitStatisticsProvider`]
/// 7. [`UnionStatisticsProvider`]
+ #[deprecated(
+ since = "56.0.0",
+ note = "the bundled providers are deprecated; the statistics walk uses
each operator's `statistics_from_inputs` when no provider matches"
+ )]
+ #[expect(deprecated)]
Review Comment:
👍 looks good. Not sure which tickets are already open for this, but what
comes to mind is:
1. Fold the improvements brought by these providers into the built-in
`partition_statistics` method.
2. Remove the now redundant providers
##########
datafusion/sqllogictest/src/test_context.rs:
##########
@@ -144,9 +149,31 @@ impl TestContext {
relative_path.file_name().and_then(|name| name.to_str()),
Some("statistics_registry.slt")
) {
- state_builder = state_builder.with_statistics_registry(
- StatisticsRegistry::default_with_builtin_providers(),
+ // Replaces the join estimate with the Cartesian product
+ let join_provider = ClosureStatisticsProvider::with_matches(
+ |plan| plan.downcast_ref::<HashJoinExec>().is_some(),
Review Comment:
I see this is still using a `ClosureStatisticsProvider`. I see this is
mainly just for testing that the registry still works and actually listens to
what the `ClosureStatisticsProvider` has to say.
If it's just that, all good, but let me know if missed something.
--
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]
