kosiew commented on code in PR #23651:
URL: https://github.com/apache/datafusion/pull/23651#discussion_r3680844319
##########
datafusion/physical-plan/src/statistics.rs:
##########
@@ -182,35 +253,233 @@ 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 {
+ if !provider.matches(plan) {
Review Comment:
Could we preserve the existing `Delegate` contract without requiring every
provider to implement `matches()`?
The new `matches()` hook avoids child-stat resolution when a provider
explicitly returns `false`. However, its default is `true`, so existing
providers that inspect `plan` inside `compute_statistics` and return `Delegate`
for unsupported nodes still resolve their child-stat requests first.
That means an unrelated provider can trigger the default `At(None)` child
walk and fail before a later provider that requests `ChildStats::Skip`, or
before the operator fallback, gets a chance to handle the node.
`ClosureStatisticsProvider::new` appears to have the same issue because it uses
an always-true matcher.
A lazy child-stat design may help here, so child statistics are only
computed once the provider actually needs them. Please also add a regression
test where a provider that does not override `matches()` returns `Delegate`,
comes before a child-skipping provider, and has an erroring child. The later
provider should still succeed.
--
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]
