Repository navigation
perf: share one StatisticsContext across all physical optimizer rules #26094
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
3179bd9
3dd8da1
784fca0
ae341dc
5a4626e
c101751
63aa427
f7587af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,14 +25,15 @@ use datafusion_physical_plan::aggregates::{ | |
| }; | ||
| use datafusion_physical_plan::placeholder_row::PlaceholderRowExec; | ||
| use datafusion_physical_plan::projection::{ProjectionExec, ProjectionExpr}; | ||
| use datafusion_physical_plan::statistics::{StatisticsArgs, StatisticsContext}; | ||
| use datafusion_physical_plan::statistics::StatisticsArgs; | ||
| use datafusion_physical_plan::udaf::{ | ||
| AggregateFunctionExpr, StatisticsArgs as PlanStatisticsArgs, | ||
| }; | ||
| use datafusion_physical_plan::{ExecutionPlan, expressions}; | ||
| use std::sync::Arc; | ||
|
|
||
| use crate::PhysicalOptimizerRule; | ||
| use crate::optimizer::{ConfigOnlyContext, PhysicalOptimizerContext}; | ||
|
|
||
| /// Optimizer that uses available statistics for aggregate functions | ||
| #[derive(Default, Debug)] | ||
|
|
@@ -46,20 +47,26 @@ impl AggregateStatistics { | |
| } | ||
|
|
||
| impl PhysicalOptimizerRule for AggregateStatistics { | ||
| #[cfg_attr(feature = "recursive_protection", recursive::recursive)] | ||
| #[expect(clippy::allow_attributes)] // See https://github.com/apache/datafusion/issues/18881#issuecomment-3621545670 | ||
| #[allow(clippy::only_used_in_recursion)] // See https://github.com/rust-lang/rust-clippy/issues/14566 | ||
| fn optimize( | ||
| &self, | ||
| plan: Arc<dyn ExecutionPlan>, | ||
| config: &ConfigOptions, | ||
| ) -> Result<Arc<dyn ExecutionPlan>> { | ||
| self.optimize_with_context(plan, &ConfigOnlyContext::new(config)) | ||
| } | ||
|
|
||
| #[cfg_attr(feature = "recursive_protection", recursive::recursive)] | ||
| fn optimize_with_context( | ||
| &self, | ||
| plan: Arc<dyn ExecutionPlan>, | ||
| context: &dyn PhysicalOptimizerContext, | ||
| ) -> Result<Arc<dyn ExecutionPlan>> { | ||
| if let Some(partial_agg_exec) = take_optimizable(&plan) { | ||
| let partial_agg_exec = partial_agg_exec | ||
| .downcast_ref::<AggregateExec>() | ||
| .expect("take_optimizable() ensures that this is a AggregateExec"); | ||
| let stats = StatisticsContext::new() | ||
| .compute(partial_agg_exec.input().as_ref(), &StatisticsArgs::new())?; | ||
| let stats = context | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This lets registered providers decide query results, not just plan choices: Test for /// `AggregateStatistics` trusts an `Exact` row count from a registered
/// provider: it answers `COUNT(*)` without scanning
#[tokio::test]
async fn aggregate_statistics_consults_statistics_providers() -> Result<()> {
use datafusion_common::stats::Precision;
use datafusion_datasource::source::DataSourceExec;
use datafusion_physical_plan::operator_statistics::{
ClosureStatisticsProvider, StatisticsResult,
};
let provider = ClosureStatisticsProvider::with_matches(
|plan| plan.is::<DataSourceExec>(),
|plan, child_stats| {
let child_stats = child_stats
.iter()
.map(|c| Arc::clone(c.base_arc()))
.collect::<Vec<_>>();
let mut stats = Arc::unwrap_or_clone(
plan.statistics_from_inputs(&child_stats, &StatisticsArgs::new())?,
);
stats.num_rows = Precision::Exact(42);
Ok(StatisticsResult::Computed(stats.into()))
},
);
let state = SessionStateBuilder::new()
.with_default_features()
.with_statistics_registry(StatisticsRegistry::with_providers(vec![Arc::new(
provider,
)]))
.build();
let ctx = SessionContext::new_with_state(state);
ctx.sql("CREATE TABLE t AS VALUES (1), (2), (3)")
.await?
.collect()
.await?;
let batches = ctx.sql("SELECT COUNT(*) FROM t").await?.collect().await?;
assert_batches_eq!(
&[
"+----------+",
"| count(*) |",
"+----------+",
"| 42 |",
"+----------+"
],
&batches
);
Ok(())
}State the contract where provider authors will read it ( /// Implementations can handle specific operator types or override default
/// estimation logic. The chain of providers is traversed until one returns
/// [`StatisticsResult::Computed`].
+///
+/// `Exact` statistics are trusted for correctness, not just cost: optimizer
+/// rules use them to answer `COUNT(*)`, `MIN` and `MAX` without scanning and
+/// to remove limits. Return `Inexact` for anything that is an estimate.And in `JoinSelection` already did. Sessions without registered providers (the
-default) are not affected.
+default) are not affected. Provider statistics can now change query results:
+`AggregateStatistics` answers `COUNT(*)`, `MIN` and `MAX` from `Exact` values,
+and `LimitPushdown` removes a limit over an `Exact(0)` input, so a provider
+must return `Exact` only for guaranteed values. |
||
| .compute_statistics(partial_agg_exec.input(), &StatisticsArgs::new())?; | ||
| let mut projections = vec![]; | ||
| for expr in partial_agg_exec.aggr_expr() { | ||
| let field = expr.field(); | ||
|
|
@@ -92,13 +99,17 @@ impl PhysicalOptimizerRule for AggregateStatistics { | |
| )?)) | ||
| } else { | ||
| plan.map_children(|child| { | ||
| self.optimize(child, config).map(Transformed::yes) | ||
| self.optimize_with_context(child, context) | ||
| .map(Transformed::yes) | ||
| }) | ||
| .data() | ||
| } | ||
| } else { | ||
| plan.map_children(|child| self.optimize(child, config).map(Transformed::yes)) | ||
| .data() | ||
| plan.map_children(|child| { | ||
| self.optimize_with_context(child, context) | ||
| .map(Transformed::yes) | ||
| }) | ||
| .data() | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1063,10 +1063,8 @@ struct PlanSize { | |
| } | ||
|
|
||
| impl PlanSize { | ||
| fn from_plan(plan: &dyn ExecutionPlan) -> Self { | ||
| let stats = StatisticsContext::new() | ||
| .compute(plan, &StatisticsArgs::new()) | ||
| .ok(); | ||
| fn from_plan(plan: &Arc<dyn ExecutionPlan>, stats_ctx: &StatisticsContext) -> Self { | ||
| let stats = stats_ctx.compute_arc(plan, &StatisticsArgs::new()).ok(); | ||
| Self { | ||
| byte_size: stats | ||
| .as_ref() | ||
|
|
@@ -1124,6 +1122,7 @@ fn enforce_distribution_relationships( | |
| input_distributions: &InputDistributionRequirements, | ||
| children: &mut [DistributionChildState], | ||
| target_partitions: usize, | ||
| stats_ctx: &StatisticsContext, | ||
| ) -> Result<()> { | ||
| let mut repartitioned_for_relationship = vec![false; children.len()]; | ||
|
|
||
|
|
@@ -1162,53 +1161,52 @@ fn enforce_distribution_relationships( | |
| } | ||
| } | ||
|
|
||
| let best_satisfied_child: Option<(usize, Partitioning)> = match satisfied_children | ||
| .len() | ||
| { | ||
| 0 => None, | ||
| 1 => satisfied_children | ||
| .into_iter() | ||
| .next() | ||
| .map(|(i, p, _)| (i, p)), | ||
| _ => { | ||
| // Prefer native partitioned children over newly repartitioned ones | ||
| let native_children: Vec<_> = satisfied_children | ||
| .iter() | ||
| .filter(|(_, _, is_native)| *is_native) | ||
| .collect(); | ||
| if native_children.len() == 1 { | ||
| let (i, p, _) = native_children[0]; | ||
| Some((*i, p.clone())) | ||
| } else { | ||
| let pool = if !native_children.is_empty() { | ||
| native_children | ||
| } else { | ||
| satisfied_children.iter().collect() | ||
| }; | ||
| let candidates: Vec<_> = pool | ||
| .into_iter() | ||
| .map(|(idx, part, _)| { | ||
| let size = | ||
| PlanSize::from_plan(children[*idx].context.plan.as_ref()); | ||
| (size, *idx, part.clone()) | ||
| }) | ||
| .collect(); | ||
|
|
||
| // Prefer a unique, strictly larger winner (`size_a > size_b`). | ||
| // Otherwise, fall back to standard distribution rather | ||
| // than choosing an arbitrary reference. | ||
| candidates | ||
| let best_satisfied_child: Option<(usize, Partitioning)> = | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note to reviewers: re-indented by |
||
| match satisfied_children.len() { | ||
| 0 => None, | ||
| 1 => satisfied_children | ||
| .into_iter() | ||
| .next() | ||
| .map(|(i, p, _)| (i, p)), | ||
| _ => { | ||
| // Prefer native partitioned children over newly repartitioned ones | ||
| let native_children: Vec<_> = satisfied_children | ||
| .iter() | ||
| .find(|(size_a, idx_a, _)| { | ||
| size_a.is_known() | ||
| && candidates.iter().all(|(size_b, idx_b, _)| { | ||
| idx_a == idx_b || size_a > size_b | ||
| }) | ||
| }) | ||
| .map(|(_, idx, part)| (*idx, part.clone())) | ||
| .filter(|(_, _, is_native)| *is_native) | ||
| .collect(); | ||
| if native_children.len() == 1 { | ||
| let (i, p, _) = native_children[0]; | ||
| Some((*i, p.clone())) | ||
| } else { | ||
| let pool = if !native_children.is_empty() { | ||
| native_children | ||
| } else { | ||
| satisfied_children.iter().collect() | ||
| }; | ||
| let candidates: Vec<_> = pool | ||
| .into_iter() | ||
| .map(|(idx, part, _)| { | ||
| let plan = &children[*idx].context.plan; | ||
| let size = PlanSize::from_plan(plan, stats_ctx); | ||
| (size, *idx, part.clone()) | ||
| }) | ||
| .collect(); | ||
|
|
||
| // Prefer a unique, strictly larger winner (`size_a > size_b`). | ||
| // Otherwise, fall back to standard distribution rather | ||
| // than choosing an arbitrary reference. | ||
| candidates | ||
| .iter() | ||
| .find(|(size_a, idx_a, _)| { | ||
| size_a.is_known() | ||
| && candidates.iter().all(|(size_b, idx_b, _)| { | ||
| idx_a == idx_b || size_a > size_b | ||
| }) | ||
| }) | ||
| .map(|(_, idx, part)| (*idx, part.clone())) | ||
| } | ||
| } | ||
| } | ||
| }; | ||
| }; | ||
|
|
||
| // Validate that best_satisfied_child can be adapted across all unsatisfied children in the group | ||
| let best_satisfied_child = | ||
|
|
@@ -1666,6 +1664,7 @@ pub fn ensure_distribution_with_stats( | |
| &input_distributions, | ||
| &mut children, | ||
| target_partitions, | ||
| stats_ctx, | ||
| )?; | ||
|
|
||
| let children = children | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing this widens, worth stating somewhere: the cache is now exposed to in-place mutation for the whole planning run, not just one rule.
A pointer-keyed hit is only valid because a plan node never changes behind its
Arc. That held trivially when each rule had its own context — anything a rule did produced new nodes. Now an entry computed by the first rule is still served to the last one, so a node that mutated its own statistics through interior mutability without changing identity would be read stale.Nothing does that today (planning-time rules all rebuild nodes, and dynamic filters are updated at execution time, after this context is dropped), so this isn't a bug — but it's an invariant the design now leans on much harder. A line on
StatisticsContextsaying cached nodes must be immutable for the context's lifetime would make it checkable in review.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, it is an invariant the design now relies on much more. Added a paragraph in f7587af to the
StatisticsContextdocs: a plan node must not change its statistics in place while a context holds it, otherwise the cache returns stale values; optimizer rules satisfy this because they replace nodes instead of changing them.