xudong963 commented on code in PR #21815:
URL: https://github.com/apache/datafusion/pull/21815#discussion_r3165494426
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -549,7 +550,19 @@ pub trait ExecutionPlan: Any + Debug + DisplayAs + Send +
Sync {
/// If statistics are not available, should return
[`Statistics::new_unknown`]
/// (the default), not an error.
/// If `partition` is `None`, it returns statistics for the entire plan.
- fn partition_statistics(&self, partition: Option<usize>) ->
Result<Arc<Statistics>> {
+ ///
+ /// The [`StatisticsContext`] carries pre-computed child statistics and
+ /// additional context for statistics computation. Use
+ /// [`compute_statistics`] to compute statistics bottom-up, threading
+ /// child statistics through the context automatically.
+ ///
+ /// [`StatisticsContext`]: crate::statistics_context::StatisticsContext
+ /// [`compute_statistics`]: crate::statistics_context::compute_statistics
+ fn partition_statistics(
Review Comment:
We should keep the old API, and add a new one
https://datafusion.apache.org/contributor-guide/api-health.html#what-is-the-public-api-and-what-is-a-breaking-api-change
##########
datafusion/physical-plan/src/statistics_context.rs:
##########
@@ -0,0 +1,102 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Context for computing statistics in physical plans.
+//!
+//! [`StatisticsContext`] provides external context to
+//! [`ExecutionPlan::partition_statistics`], enabling operators
+//! to receive pre-computed child statistics and additional context for
+//! statistics computation.
+
+use crate::ExecutionPlan;
+use datafusion_common::Result;
+use datafusion_common::Statistics;
+use std::sync::Arc;
+
+/// Context passed to [`ExecutionPlan::partition_statistics`]
+/// carrying external information that operators can use when computing
+/// their statistics.
+///
+/// # Example
+///
+/// ```ignore
+/// use datafusion_physical_plan::statistics_context::StatisticsContext;
+///
+/// // Leaf node: no children
+/// let leaf_ctx = StatisticsContext::empty();
+///
+/// // Parent node: receives pre-computed child stats
+/// let child_stats = vec![child1_stats, child2_stats];
+/// let parent_ctx = StatisticsContext::new(child_stats);
+/// ```
+#[derive(Debug, Clone)]
+pub struct StatisticsContext {
+ /// Pre-computed statistics for each child of the current node,
+ /// in the same order as [`ExecutionPlan::children`].
+ child_stats: Vec<Arc<Statistics>>,
+}
+
+impl StatisticsContext {
+ /// Creates a new context with pre-computed child statistics.
+ pub fn new(child_stats: Vec<Arc<Statistics>>) -> Self {
+ Self { child_stats }
+ }
+
+ /// Creates an empty context (for leaf nodes or when child stats
+ /// are not available).
+ pub fn empty() -> Self {
+ Self {
+ child_stats: Vec::new(),
+ }
+ }
+
+ /// Returns the pre-computed statistics for each child node.
+ pub fn child_stats(&self) -> &[Arc<Statistics>] {
+ &self.child_stats
+ }
+}
+
+impl Default for StatisticsContext {
+ fn default() -> Self {
+ Self::empty()
+ }
+}
+
+/// Computes statistics for a plan node by first recursively computing
+/// statistics for all children, then calling
+/// [`ExecutionPlan::partition_statistics`] with the pre-computed child
+/// statistics.
+///
+/// The `partition` parameter is forwarded to children. This is correct
+/// for partition-preserving operators (filter, projection, sort, etc.),
+/// but operators that need overall child stats regardless of the
+/// requested partition (e.g., repartition, coalesce, asymmetric joins)
+/// must handle this internally by calling `compute_statistics` with
+/// `None` on the relevant children.
+pub fn compute_statistics(
+ plan: &dyn ExecutionPlan,
+ partition: Option<usize>,
+) -> Result<Arc<Statistics>> {
+ let child_stats = plan
+ .children()
+ .iter()
+ .map(|child| compute_statistics(child.as_ref(), partition))
Review Comment:
`compute_statistics` always recurses with the same `partition`. For
partition-merging operators this is wasted work because they'll discard the
context and recompute with `None` anyway
--
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]
