Skip to content

Commit dcd385e

Browse files
authored
perf: reuse one StatisticsContext across ensure_distribution (#25098)
## Which issue does this PR close? None filed; small self-contained perf fix. Rationale below. ## Rationale for this change `get_repartition_requirement_status` creates a fresh `StatisticsContext::new()` **once per child**. `StatisticsContext::compute` recurses the child's whole subtree and carries a pointer-keyed memoization cache its own docstring describes as a *"per-call memoization cache"* meant to be reused across a walk. Allocating a new context per child discards that cache every time, so a single `ensure_distribution` pass recomputes shared subtree statistics `O(depth)` times. On a deep/wide plan this is measurable. In our deployment (`EnsureRequirements` runs several times over a ~200-node plan) sharing the cache cut physical planning by ~10% with no plan change. ## What changes are included in this PR? - Thread one `StatisticsContext` through the `ensure_distribution` `transform_up` (pass `&StatisticsContext` into `get_repartition_requirement_status`) so each subtree's statistics are computed once per pass. - `StatsCache` is keyed by raw node pointer, and `ensure_distribution` returns `Transformed::yes` unconditionally, so the cache reset is keyed on whether the node's plan pointer **actually changed** (`Arc::ptr_eq` before/after). A node that changed may have freed a cached child (which would make a stale pointer key unsafe); a node that made no change cannot, so the cache safely persists across the no-op nodes that dominate a deep plan. (A second per-child `StatisticsContext::new()` in `PlanSize::from_plan` / `enforce_distribution_relationships` can get the same treatment; left as a follow-up to keep this PR focused.) ## Are these changes tested? Yes. New test `ensure_distribution_shares_statistics_cache` puts a leaf that counts its own statistics computations under a stack of pass-through operators, runs the distribution pass with a shared context vs a fresh-per-node context, and asserts the shared cache saves progressively more as the stack deepens. A cache that is not actually shared (e.g. reset on every node) saves nothing and fails the test — which a plan-output assertion cannot catch, since the optimized plan is identical either way. Existing suites remain green and unchanged: `datafusion --test core_integration physical_optimizer` (569 passed) and `datafusion-physical-plan` statistics tests (96 passed). ## Are there any user-facing changes? No. Internal physical-optimizer performance only; planner output is identical.
1 parent 4e90755 commit dcd385e

4 files changed

Lines changed: 382 additions & 10 deletions

File tree

‎datafusion/core/tests/physical_optimizer/enforce_distribution.rs‎

Lines changed: 280 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
use std::fmt::Debug;
1919
use std::ops::Deref;
2020
use std::sync::Arc;
21+
use std::sync::atomic::{AtomicUsize, Ordering};
2122

2223
use crate::physical_optimizer::test_utils::{
2324
RequirementsTestExec, bounded_window_exec_with_can_repartition, check_integrity,
@@ -56,6 +57,7 @@ use datafusion_physical_expr_common::physical_expr::PhysicalExpr;
5657
use datafusion_physical_expr_common::sort_expr::{
5758
LexOrdering, OrderingRequirements, PhysicalSortExpr,
5859
};
60+
use datafusion_physical_optimizer::PhysicalOptimizerContext;
5961
use datafusion_physical_optimizer::PhysicalOptimizerRule;
6062
use datafusion_physical_optimizer::enforce_distribution::*;
6163
use datafusion_physical_optimizer::ensure_requirements::EnsureRequirements;
@@ -65,6 +67,9 @@ use datafusion_physical_optimizer::sanity_checker::SanityCheckPlan;
6567
use datafusion_physical_plan::aggregates::{
6668
AggregateExec, AggregateMode, PhysicalGroupBy,
6769
};
70+
use datafusion_physical_plan::operator_statistics::{
71+
ClosureStatisticsProvider, StatisticsRegistry, StatisticsResult,
72+
};
6873

6974
use datafusion_physical_expr::{
7075
Distribution, Partitioning, RangePartitioning, SplitPoint,
@@ -643,7 +648,12 @@ fn ensure_distribution_helper(
643648
config.optimizer.repartition_file_scans = false;
644649
config.optimizer.repartition_file_min_size = 1024;
645650
config.optimizer.prefer_existing_sort = prefer_existing_sort;
646-
ensure_distribution(distribution_context, &config).map(|item| item.data.plan)
651+
ensure_distribution_with_stats(
652+
distribution_context,
653+
&config,
654+
&datafusion_physical_plan::statistics::StatisticsContext::new(),
655+
)
656+
.map(|item| item.data.plan)
647657
}
648658

649659
fn test_suite_default_config_options() -> ConfigOptions {
@@ -766,7 +776,11 @@ impl TestConfig {
766776
// Then run ensure_distribution rule
767777
DistributionContext::new_default(adjusted)
768778
.transform_up(|distribution_context| {
769-
ensure_distribution(distribution_context, &self.config)
779+
ensure_distribution_with_stats(
780+
distribution_context,
781+
&self.config,
782+
&datafusion_physical_plan::statistics::StatisticsContext::new(),
783+
)
770784
})
771785
.data()
772786
.and_then(check_integrity)?;
@@ -5065,7 +5079,13 @@ async fn assert_reoptimized_fetch_values(
50655079
if iteration > 0 {
50665080
let distribution =
50675081
DistributionContext::new_default(Arc::clone(&optimized))
5068-
.transform_up(|context| ensure_distribution(context, &config))?
5082+
.transform_up(|context| {
5083+
ensure_distribution_with_stats(
5084+
context,
5085+
&config,
5086+
&datafusion_physical_plan::statistics::StatisticsContext::new(),
5087+
)
5088+
})?
50695089
.data;
50705090
check_integrity(distribution)?;
50715091
optimized = EnsureRequirements::new().optimize(optimized, &config)?;
@@ -5140,7 +5160,13 @@ async fn check_fetch_below_filter(
51405160
for iteration in 0..3 {
51415161
if iteration > 0 {
51425162
let distribution = DistributionContext::new_default(Arc::clone(&plan))
5143-
.transform_up(|context| ensure_distribution(context, &config))?
5163+
.transform_up(|context| {
5164+
ensure_distribution_with_stats(
5165+
context,
5166+
&config,
5167+
&datafusion_physical_plan::statistics::StatisticsContext::new(),
5168+
)
5169+
})?
51445170
.data;
51455171
check_integrity(distribution)?;
51465172
plan = EnsureRequirements::new().optimize(plan, &config)?;
@@ -5312,3 +5338,253 @@ fn ensure_distribution_reuses_plan_arc_when_no_redistribution_needed() -> Result
53125338
);
53135339
Ok(())
53145340
}
5341+
5342+
/// Single-child pass-through whose `statistics_from_inputs` increments a counter
5343+
/// every time it is actually computed (i.e. on a statistics-cache miss). Used to
5344+
/// observe how often `ensure_distribution` recomputes a node's statistics.
5345+
#[derive(Debug)]
5346+
struct CountingStatsExec {
5347+
input: Arc<dyn ExecutionPlan>,
5348+
cache: Arc<PlanProperties>,
5349+
calls: Arc<AtomicUsize>,
5350+
}
5351+
5352+
impl CountingStatsExec {
5353+
fn new(input: Arc<dyn ExecutionPlan>, calls: Arc<AtomicUsize>) -> Self {
5354+
let cache = PlanProperties::new(
5355+
input.equivalence_properties().clone(),
5356+
input.output_partitioning().clone(),
5357+
input.pipeline_behavior(),
5358+
input.boundedness(),
5359+
);
5360+
Self {
5361+
input,
5362+
cache: Arc::new(cache),
5363+
calls,
5364+
}
5365+
}
5366+
}
5367+
5368+
impl DisplayAs for CountingStatsExec {
5369+
fn fmt_as(
5370+
&self,
5371+
_t: DisplayFormatType,
5372+
f: &mut std::fmt::Formatter,
5373+
) -> std::fmt::Result {
5374+
write!(f, "CountingStatsExec")
5375+
}
5376+
}
5377+
5378+
impl ExecutionPlan for CountingStatsExec {
5379+
fn name(&self) -> &'static str {
5380+
"CountingStatsExec"
5381+
}
5382+
5383+
fn properties(&self) -> &Arc<PlanProperties> {
5384+
&self.cache
5385+
}
5386+
5387+
fn children(&self) -> Vec<&Arc<dyn ExecutionPlan>> {
5388+
vec![&self.input]
5389+
}
5390+
5391+
fn replace_children(
5392+
self: Arc<Self>,
5393+
mut children: Vec<Arc<dyn ExecutionPlan>>,
5394+
_: ReplaceChildrenOptions,
5395+
) -> Result<Arc<dyn ExecutionPlan>> {
5396+
assert_eq!(children.len(), 1);
5397+
Ok(Arc::new(Self::new(
5398+
children.pop().unwrap(),
5399+
Arc::clone(&self.calls),
5400+
)))
5401+
}
5402+
5403+
fn with_new_children(
5404+
self: Arc<Self>,
5405+
children: Vec<Arc<dyn ExecutionPlan>>,
5406+
) -> Result<Arc<dyn ExecutionPlan>> {
5407+
self.replace_children(
5408+
children,
5409+
ReplaceChildrenOptions::new(ChildrenPropertiesMode::Recompute),
5410+
)
5411+
}
5412+
5413+
fn apply_expressions(
5414+
&self,
5415+
_f: &mut dyn FnMut(&Arc<dyn PhysicalExpr>) -> Result<TreeNodeRecursion>,
5416+
) -> Result<TreeNodeRecursion> {
5417+
Ok(TreeNodeRecursion::Continue)
5418+
}
5419+
5420+
fn execute(
5421+
&self,
5422+
_partition: usize,
5423+
_context: Arc<datafusion::execution::context::TaskContext>,
5424+
) -> Result<datafusion_physical_plan::SendableRecordBatchStream> {
5425+
unreachable!();
5426+
}
5427+
5428+
fn statistics_from_inputs(
5429+
&self,
5430+
_input_stats: &[Arc<Statistics>],
5431+
_args: &datafusion_physical_plan::statistics::StatisticsArgs,
5432+
) -> Result<Arc<Statistics>> {
5433+
self.calls.fetch_add(1, Ordering::Relaxed);
5434+
Ok(Arc::new(Statistics::new_unknown(
5435+
self.input.schema().as_ref(),
5436+
)))
5437+
}
5438+
}
5439+
5440+
/// Regression test for the shared statistics cache in `ensure_distribution`.
5441+
///
5442+
/// A deep stack of pass-through operators sits over a counting leaf. Each
5443+
/// ancestor's distribution enforcement inspects its child's statistics, which
5444+
/// recurse to the leaf. With one `StatisticsContext` shared across the pass the
5445+
/// leaf is computed once; with a fresh context per node it is recomputed once
5446+
/// per ancestor. This directly detects a regression where the cache is not
5447+
/// actually shared (e.g. reset on every node), which no plan-output assertion
5448+
/// can catch because the optimized plan is identical either way.
5449+
#[test]
5450+
fn ensure_distribution_shares_statistics_cache() -> Result<()> {
5451+
// Count how many times a leaf's statistics are computed over a stack of
5452+
// `depth` pass-through operators sitting on top of it. Each ancestor's
5453+
// distribution enforcement inspects its child's statistics, which recurse to
5454+
// the leaf.
5455+
//
5456+
// The measured arm drives the real `EnsureRequirements` rule, so the sharing
5457+
// and the cache-reset condition under test are the ones the rule actually
5458+
// uses — reimplementing them here would keep passing even if the rule
5459+
// stopped sharing. The baseline arm allocates a fresh `StatisticsContext`
5460+
// per node, reproducing the behavior before this change.
5461+
fn deep_plan(depth: usize, calls: &Arc<AtomicUsize>) -> Arc<dyn ExecutionPlan> {
5462+
let mut plan: Arc<dyn ExecutionPlan> =
5463+
Arc::new(CountingStatsExec::new(parquet_exec(), Arc::clone(calls)));
5464+
for _ in 0..depth {
5465+
plan = filter_exec(plan);
5466+
}
5467+
plan
5468+
}
5469+
5470+
fn config() -> ConfigOptions {
5471+
let mut config = ConfigOptions::new();
5472+
config.execution.target_partitions = 10;
5473+
// Keep the plan a fixpoint so no node is rebuilt and the shared cache is
5474+
// never reset; statistics are still computed for the round-robin decision.
5475+
config.optimizer.enable_round_robin_repartition = false;
5476+
config
5477+
}
5478+
5479+
/// Leaf statistics computations performed by the real rule.
5480+
fn via_rule(depth: usize) -> Result<usize> {
5481+
let calls = Arc::new(AtomicUsize::new(0));
5482+
EnsureRequirements::new().optimize(deep_plan(depth, &calls), &config())?;
5483+
Ok(calls.load(Ordering::Relaxed))
5484+
}
5485+
5486+
/// Leaf statistics computations with a fresh context per node.
5487+
fn per_node_context(depth: usize) -> Result<usize> {
5488+
let calls = Arc::new(AtomicUsize::new(0));
5489+
let config = config();
5490+
DistributionContext::new_default(deep_plan(depth, &calls)).transform_up(
5491+
|ctx| {
5492+
ensure_distribution_with_stats(
5493+
ctx,
5494+
&config,
5495+
&datafusion_physical_plan::statistics::StatisticsContext::new(),
5496+
)
5497+
},
5498+
)?;
5499+
Ok(calls.load(Ordering::Relaxed))
5500+
}
5501+
5502+
let (shared_shallow, fresh_shallow) = (via_rule(4)?, per_node_context(4)?);
5503+
let (shared_deep, fresh_deep) = (via_rule(12)?, per_node_context(12)?);
5504+
5505+
// Sharing strictly reduces statistics recomputation at any depth. A rule that
5506+
// stopped sharing (or reset the cache on every node) would make these equal.
5507+
assert!(
5508+
shared_shallow < fresh_shallow && shared_deep < fresh_deep,
5509+
"shared cache must recompute less: shallow {shared_shallow} vs {fresh_shallow}, \
5510+
deep {shared_deep} vs {fresh_deep}"
5511+
);
5512+
5513+
// Without sharing, each extra ancestor recomputes the leaf's subtree, so the
5514+
// gap widens as the plan gets deeper. That is the depth-scaling recomputation
5515+
// the shared cache removes.
5516+
let saved_shallow = fresh_shallow - shared_shallow;
5517+
let saved_deep = fresh_deep - shared_deep;
5518+
assert!(
5519+
saved_deep > saved_shallow,
5520+
"the shared cache should save more on deeper plans: \
5521+
saved {saved_shallow} at depth 4, {saved_deep} at depth 12"
5522+
);
5523+
5524+
Ok(())
5525+
}
5526+
5527+
/// `EnsureRequirements::optimize_with_context` must thread the session's
5528+
/// statistics registry into the distribution pass, so registered providers can
5529+
/// influence cost-based decisions (here, whether a round-robin repartition is
5530+
/// worthwhile). A tiny single-partition scan does not warrant round-robin on its
5531+
/// real statistics; a provider that reports it as large flips that decision, but
5532+
/// only if the registry is actually threaded through.
5533+
#[test]
5534+
fn ensure_distribution_uses_context_statistics_registry() -> Result<()> {
5535+
let alias = vec![("a".to_string(), "a".to_string())];
5536+
let plan = aggregate_exec_with_alias(parquet_exec_with_size(1, 100), alias);
5537+
5538+
let mut config = ConfigOptions::new();
5539+
config.execution.target_partitions = 10;
5540+
// Make the round-robin decision actually depend on the estimated row count.
5541+
config
5542+
.execution
5543+
.use_row_number_estimates_to_optimize_partitioning = true;
5544+
5545+
// Default context: no registry, so the scan's real (tiny) statistics apply.
5546+
let plan_default = EnsureRequirements::new().optimize(plan.clone(), &config)?;
5547+
5548+
// A provider that reports the scan as large.
5549+
let mut registry = StatisticsRegistry::new();
5550+
registry.register(Arc::new(ClosureStatisticsProvider::with_matches(
5551+
|p| p.name() == "DataSourceExec",
5552+
|p, _child_stats| {
5553+
let mut stats = Statistics::new_unknown(&p.schema());
5554+
stats.num_rows = Precision::Inexact(10_000_000);
5555+
Ok(StatisticsResult::Computed(stats.into()))
5556+
},
5557+
)));
5558+
5559+
struct ContextWithRegistry {
5560+
config: ConfigOptions,
5561+
registry: StatisticsRegistry,
5562+
}
5563+
impl PhysicalOptimizerContext for ContextWithRegistry {
5564+
fn config_options(&self) -> &ConfigOptions {
5565+
&self.config
5566+
}
5567+
fn statistics_registry(&self) -> Option<&StatisticsRegistry> {
5568+
Some(&self.registry)
5569+
}
5570+
}
5571+
5572+
let plan_registry = EnsureRequirements::new()
5573+
.optimize_with_context(plan, &ContextWithRegistry { config, registry })?;
5574+
5575+
let s_default = displayable(plan_default.as_ref()).indent(true).to_string();
5576+
let s_registry = displayable(plan_registry.as_ref()).indent(true).to_string();
5577+
5578+
// With the scan's tiny real stats, a round-robin repartition is not worth it.
5579+
assert!(
5580+
!s_default.contains("RoundRobinBatch"),
5581+
"default context (tiny stats) should not add a round-robin repartition:\n{s_default}"
5582+
);
5583+
// The registry reports the scan as large, so the same rule now parallelizes
5584+
// it — proving the registry was threaded through `optimize_with_context`.
5585+
assert!(
5586+
s_registry.contains("RoundRobinBatch"),
5587+
"registry-reported large stats should add a round-robin repartition:\n{s_registry}"
5588+
);
5589+
Ok(())
5590+
}

0 commit comments

Comments
 (0)