asolimando commented on code in PR #23651:
URL: https://github.com/apache/datafusion/pull/23651#discussion_r3681905568
##########
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:
Thanks @kosiew, your review comments are really helpful.
Re. the lazy vs eager design: I have "inherited" the eager child statistics
resolution from #20184 and #22958 (implemented in #23051): the external
`StatisticsContext` resolves child statistics first, then hands them to
`statistics_from_inputs` (and here, to providers), so a node expresses only
local propagation logic and externally-computed statistics (external catalogs,
past-run feedback, ML cardinality estimators) can be injected through the same
path.
I have considered a lazy design as an alternative but I didn't pursue it
since the eager design was already backed by multiple maintainers in the
issues/PRs linked above, after extensive discussion, so I considered the
decision hard to reconsider.
The fatal-error problem you raise is important, but there is a middle ground
solution to handle that in the eager design: if a provider's child walk fails,
skip that provider and let a later one or the operator fallback handle the node.
Concretely, it would read as something like this:
```rust
let child_statistics = match self.resolve_children(plan, children,
&requests) {
Ok(child_statistics) => child_statistics,
Err(e) => {
debug!("Statistics provider {provider:?} skipped for {}: ... {e}",
...);
continue;
}
};
```
Note that this isn't error-swallowing: if the child is genuinely needed even
by the built-in fallback, the error resurfaces there. A matched provider's own
`compute` error stays fatal (which seems fair, and matches similar patterns
like physical rules). Today each provider is responsible for its own
recoverable errors: it can return `Delegate` for transient failures (e.g.,
network calls to an external catalog) and surface `Err` only for hard cases. If
you prefer, we could instead catch compute errors the same way as the child
walk.
I have optimistically implemented this solution in f9355ca67 to ground the
discussion (along with the regression test you asked for), happy to take a
different route if you prefer.
I think this keeps the good sides of both designs: robust over errors,
doesn't go against previously agreed eager evaluation for statistics.
In terms of performance I think we are safe too, there is enough machinery
to prevent waste for providers:
- `matches()` can easily express operator-level applicability which is
probably the vast majority of cases
- `Delegate` in `compute()` for finer-grained checks (e.g., I handle
`ProjectionExec` but only a subset of expressions, and delegate on the rest)
- `ChildStats::Skip` for avoiding computing unneeded stats for some children
- caching via `StatisticsContext` so repeated stats computation across
providers/built-in doesn't trigger multiple walks
What do you think?
--
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]
