kosiew commented on code in PR #23651:
URL: https://github.com/apache/datafusion/pull/23651#discussion_r3683436190
##########
datafusion/physical-plan/src/statistics.rs:
##########
@@ -182,35 +255,254 @@ 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))
- }
+ .enumerate()
+ .map(|(i, (child, directive))| match directive {
+ ChildStats::At(p) => self
+ .compute_base(child.as_ref(),
&StatisticsArgs::new().with_partition(*p))
+ .map_err(|e| {
+ e.context(format!(
+ "computing statistics for child {i} ({}) of {} at
partition {p:?}",
+ child.name(),
+ plan.name()
+ ))
+ }),
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) {
+ continue;
Review Comment:
I like the delegate-chain fix, but I think we're now skipping too much.
At the moment, every `resolve_children(...)` error falls into the same
`continue` path. That means provider contract violations and other hard
failures are treated the same as delegation failures, which can silently
disable provider behavior and make bugs much harder to spot.
Could we narrow the classification here so that only delegation-related
failures are skipped? I'd keep the current eager resolution model, but continue
to fail fast for provider contract violations, especially request-length
mismatches, 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]
