asolimando commented on code in PR #23651:
URL: https://github.com/apache/datafusion/pull/23651#discussion_r3685161146
##########
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:
Thanks @kosiew, I agree that for invalid provider requests (I added
out-of-bounds partitions to the malformed request length you mentioned) it’s
good to fail fast.
To that effect, in 08845ef8a I've split validation from resolution:
validating the provider's requests now fails fast, while a child-statistics
computation error during resolution is passed on to the next provider (or the
operator fallback).
I have added two regression tests to verify that both a wrong-length request
set and an out-of-range partition request fail fast and don’t go to the next
provider.
--
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]
