kosiew commented on code in PR #23651:
URL: https://github.com/apache/datafusion/pull/23651#discussion_r3663175978
##########
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:
Could we defer resolving a provider's `child_stats_requests` until we know
that the provider will handle the current node?
Right now, `try_provider_stats` resolves each provider's child statistics
request before calling that provider. As a result, a provider that eventually
returns `Delegate` can still force child statistics to be computed. If that
child walk fails, the next provider or the operator fallback never gets a
chance to run.
The current regression test covers one matching provider that requests
`ChildStats::Skip`, but it does not cover a provider chain. For example,
provider A could use the default `child_stats_requests` value of `At(None)` and
then delegate, while provider B would handle the node using `Skip`. Provider
A's child walk can fail before provider B is called. The same issue affects an
operator fallback whose `ExecutionPlan::child_stats_requests` is `Skip`, since
registering an unrelated provider can unexpectedly cause all child statistics
to be computed.
An applicability hook such as `supports(plan)` could be checked before
resolving child statistics. An equivalent lazy design would also work, as long
as child statistics are only resolved when the provider actually computes the
result.
Please also add a regression test with a delegating provider before a `Skip`
provider and an erroring child, then assert that the later provider still
succeeds.
--
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]
