asolimando commented on code in PR #23651:
URL: https://github.com/apache/datafusion/pull/23651#discussion_r3665430333
##########
datafusion/physical-plan/src/statistics.rs:
##########
@@ -182,35 +253,230 @@ impl StatisticsContext {
requests.len(),
children.len()
);
- let child_stats = children
+ children
.iter()
.zip(requests)
.map(|(child, directive)| match directive {
- ChildStats::At(p) => {
- self.compute(child.as_ref(),
&StatisticsArgs::new().with_partition(p))
- }
+ ChildStats::At(p) => self.compute_base(
+ child.as_ref(),
+ &StatisticsArgs::new().with_partition(*p),
+ ),
ChildStats::Skip => {
Ok(Arc::new(Statistics::new_unknown(child.schema().as_ref())))
}
})
- .collect::<Result<Vec<_>>>()?;
+ .collect()
+ }
+
+ /// Runs the provider chain, returning the first `Computed` result's core
+ /// statistics (and recording its extensions), or `None` if the chain is
empty
+ /// or all delegate. A partition-blind provider applies only to overall
stats
+ /// (its default `compute_statistics_with_args` delegates per partition).
+ ///
+ /// Each provider's child statistics come from its own
+ ///
[`child_stats_requests`](crate::operator_statistics::StatisticsProvider::child_stats_requests)
+ /// and are memoized, so a walk with no providers pays nothing.
+ fn try_provider_stats(
+ &self,
+ plan: &dyn ExecutionPlan,
+ children: &[&Arc<dyn ExecutionPlan>],
+ args: &StatisticsArgs,
+ ) -> Result<Option<Arc<Statistics>>> {
+ let providers = self.registry.providers();
+ if providers.is_empty() {
+ return Ok(None);
+ }
+ 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)?;
Review 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.
--
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]
