kosiew commented on code in PR #23651:
URL: https://github.com/apache/datafusion/pull/23651#discussion_r3637806643
##########
datafusion/physical-plan/src/statistics.rs:
##########
@@ -168,12 +216,32 @@ impl StatisticsContext {
);
}
- if let Some(cached) = self.cache.borrow().get(plan, partition) {
- return Ok(Arc::clone(cached));
+ if let Some(cached) = self.cached_statistics(plan, partition) {
+ return Ok(cached);
}
let children = plan.children();
let requests = plan.child_stats_requests(partition);
+ // Resolved for the built-in `statistics_from_inputs` fallback below.
+ let child_statistics = self.resolve_children(plan, &children,
&requests)?;
Review Comment:
Could we try the provider path before resolving the operator's child
statistics requests?
Right now, `compute_base` calls `plan.child_stats_requests()` before trying
any provider. This means an operator's fallback-only child walk still runs even
when a matching provider requests `ChildStats::Skip` and can compute the node's
statistics itself. Besides doing unnecessary work, an error from that child
walk prevents the provider from overriding the node.
The existing `test_provider_opts_out_of_child_stats` confirms that the
provider receives an unknown placeholder, but it does not confirm that the
child computation was actually skipped.
Please resolve each provider's own requests while trying that provider, then
resolve the operator's requests only after every provider delegates,
immediately before calling `statistics_from_inputs`. It would also be helpful
to add a regression test where the operator requests `At(...)`, the matching
provider requests `Skip`, and the test asserts that the child is never computed.
##########
datafusion/physical-plan/src/operator_statistics/mod.rs:
##########
@@ -346,52 +411,39 @@ impl StatisticsRegistry {
&self.providers
}
- /// Compute extended statistics for a plan through the provider chain.
+ /// Compute extended statistics for `plan` through the provider chain.
///
- /// Performs a bottom-up tree walk: child statistics are computed
recursively
- /// and passed to providers, mirroring how `partition_statistics` composes
- /// operators. Once
[#20184](https://github.com/apache/datafusion/issues/20184)
- /// lands, the registry can feed enriched base stats directly into
- /// `partition_statistics(child_stats)`, removing the need for a separate
walk.
+ /// Thin wrapper over the single [`StatisticsContext`] walk (the same path
the
+ /// optimizer and EXPLAIN use), so there is one traversal implementation.
+ /// Provider extensions are preserved; see [`StatisticsContext`] for how
they
+ /// propagate up the tree.
///
- /// If no providers are registered, falls back to the plan's built-in
- /// `partition_statistics(None)` with no overhead.
+ /// Children are resolved via [`ExecutionPlan::child_stats_requests`]
(default
Review Comment:
Could we update or remove this paragraph? It still says child statistics are
resolved through `ExecutionPlan::child_stats_requests` and that the operator
must declare `At`.
The follow-up moved that responsibility to
`StatisticsProvider::child_stats_requests`, which allows providers for existing
operators to request child statistics without changing those operators. Since
this deprecated wrapper now follows the provider-specific behavior, the rustdoc
should describe that behavior instead.
--
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]
