kosiew commented on code in PR #23651:
URL: https://github.com/apache/datafusion/pull/23651#discussion_r3683409047
##########
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, this explanation is very helpful and I agree with keeping the eager
model for now.
I also agree with your middle-ground goal (do not let an early delegating
provider block later providers or fallback), and the new regression test for
that scenario is great.
I still see one remaining contract risk in the current implementation: all
provider child-resolution errors are treated as skippable. That can mask
provider bugs or contract violations (for example, malformed
child_stats_requests) and silently disable provider logic.
Could we narrow the skip behavior to only the delegation-related failure
mode, while still surfacing hard/provider-contract failures? In other words:
1. keep the resilience benefit for the delegate-chain case
2. preserve fail-fast behavior for invalid provider requests and other hard
errors
--
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]
