Skip to content

Commit 5b65e70

Browse files
authored
Cap SortPreservingMerge statistics by fetch (apache#23359)
## Which issue does this PR close? - Closes none. ## Rationale for this change `SortPreservingMergeExec` supports `fetch`, but its statistics currently pass through child statistics unchanged. This can make cost-based optimizers overestimate top-k merge outputs. For example, a `SortPreservingMergeExec(fetch=1)` side can still look as large as its input to `JoinSelection`, causing hash join build/probe choices to miss the small top-k side. Other fetch-aware operators such as `SortExec`, `CoalescePartitionsExec`, and limit execs already cap their statistics by fetch. This PR aligns `SortPreservingMergeExec` with that behavior. ## What changes are included in this PR? - Cap `SortPreservingMergeExec` statistics with `Statistics::with_fetch(self.fetch, 0, 1)`. - Preserve no-op statistics for `fetch = None` and `skip = 0`. - Avoid scaling byte-size estimates by `0.0` when input row count is unknown; use absent byte-size stats instead. - Add regression tests for SPM fetch statistics and for `JoinSelection` swapping an SPM(fetch=1) side to the hash join build side. ## Are these changes tested? Yes. - `cargo test -p datafusion-common test_with_fetch` - `cargo test -p datafusion-physical-plan sort_preserving_merge` - `cargo test -p datafusion join_selection --test core_integration` - `cargo check -p datafusion --test core_integration` - `cargo fmt --all -- --check` - `cargo clippy --all-targets --all-features -- -D warnings` ## Are there any user-facing changes? No user-facing API changes. This improves optimizer statistics for fetch-limited sort-preserving merge plans.
1 parent d75742e commit 5b65e70

3 files changed

Lines changed: 189 additions & 14 deletions

File tree

datafusion/common/src/stats.rs

Lines changed: 46 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -545,6 +545,10 @@ impl Statistics {
545545
skip: usize,
546546
n_partitions: usize,
547547
) -> Result<Self> {
548+
if fetch.is_none() && skip == 0 {
549+
return Ok(self);
550+
}
551+
548552
let fetch_val = fetch.unwrap_or(usize::MAX);
549553

550554
// Get the ratio of rows after / rows before on a per-partition basis
@@ -598,30 +602,30 @@ impl Statistics {
598602
..
599603
} => check_num_rows(fetch.and_then(|v| v.checked_mul(n_partitions)), false),
600604
};
601-
let ratio: f64 = match (num_rows_before, self.num_rows) {
605+
let ratio: Option<f64> = match (num_rows_before, self.num_rows) {
602606
(
603607
Precision::Exact(nr_before) | Precision::Inexact(nr_before),
604608
Precision::Exact(nr_after) | Precision::Inexact(nr_after),
605609
) => {
606610
if nr_before == 0 {
607-
0.0
611+
Some(0.0)
608612
} else {
609-
nr_after as f64 / nr_before as f64
613+
Some(nr_after as f64 / nr_before as f64)
610614
}
611615
}
612-
_ => 0.0,
616+
_ => None,
613617
};
614618
self.column_statistics = self
615619
.column_statistics
616620
.into_iter()
617621
.map(|cs| {
618622
let mut cs = cs.to_inexact();
619623
// Scale byte_size by the row ratio
620-
cs.byte_size = match cs.byte_size {
621-
Precision::Exact(n) | Precision::Inexact(n) => {
624+
cs.byte_size = match (cs.byte_size, ratio) {
625+
(Precision::Exact(n) | Precision::Inexact(n), Some(ratio)) => {
622626
Precision::Inexact((n as f64 * ratio) as usize)
623627
}
624-
Precision::Absent => Precision::Absent,
628+
_ => Precision::Absent,
625629
};
626630
// NDV can never exceed the number of rows
627631
if let Some(&rows) = self.num_rows.get_value() {
@@ -643,11 +647,11 @@ impl Statistics {
643647
Some(sum) => Precision::Inexact(sum),
644648
None => {
645649
// Fall back to scaling original total_byte_size if not all columns have byte_size
646-
match &self.total_byte_size {
647-
Precision::Exact(n) | Precision::Inexact(n) => {
650+
match (&self.total_byte_size, ratio) {
651+
(Precision::Exact(n) | Precision::Inexact(n), Some(ratio)) => {
648652
Precision::Inexact((*n as f64 * ratio) as usize)
649653
}
650-
Precision::Absent => Precision::Absent,
654+
_ => Precision::Absent,
651655
}
652656
}
653657
};
@@ -2376,6 +2380,38 @@ mod tests {
23762380
assert_eq!(result.total_byte_size, Precision::Exact(800));
23772381
}
23782382

2383+
#[test]
2384+
fn test_with_fetch_no_limit_preserves_absent_num_rows() {
2385+
let original_stats = Statistics {
2386+
num_rows: Precision::Absent,
2387+
total_byte_size: Precision::Exact(800),
2388+
column_statistics: vec![col_stats_i64(10)],
2389+
};
2390+
2391+
let result = original_stats.clone().with_fetch(None, 0, 1).unwrap();
2392+
2393+
assert_eq!(result, original_stats);
2394+
}
2395+
2396+
#[test]
2397+
fn test_with_fetch_absent_num_rows_does_not_zero_byte_size() {
2398+
let original_stats = Statistics {
2399+
num_rows: Precision::Absent,
2400+
total_byte_size: Precision::Exact(800),
2401+
column_statistics: vec![col_stats_i64(10)],
2402+
};
2403+
2404+
let result = original_stats.with_fetch(Some(1), 0, 1).unwrap();
2405+
2406+
assert_eq!(result.num_rows, Precision::Inexact(1));
2407+
assert_eq!(result.total_byte_size, Precision::Absent);
2408+
assert_eq!(result.column_statistics[0].byte_size, Precision::Absent);
2409+
assert_eq!(
2410+
result.column_statistics[0].distinct_count,
2411+
Precision::Inexact(1)
2412+
);
2413+
}
2414+
23792415
#[test]
23802416
fn test_with_fetch_with_skip() {
23812417
// Test with both skip and fetch

datafusion/core/tests/physical_optimizer/join_selection.rs

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ use datafusion_physical_expr::expressions::col;
3535
use datafusion_physical_expr::expressions::{BinaryExpr, Column, NegativeExpr};
3636
use datafusion_physical_expr::intervals::utils::check_support;
3737
use datafusion_physical_expr::{EquivalenceProperties, Partitioning, PhysicalExpr};
38+
use datafusion_physical_expr_common::sort_expr::PhysicalSortExpr;
3839
use datafusion_physical_optimizer::PhysicalOptimizerRule;
3940
use datafusion_physical_optimizer::join_selection::JoinSelection;
4041
use datafusion_physical_plan::ExecutionPlanProperties;
@@ -43,6 +44,7 @@ use datafusion_physical_plan::joins::utils::ColumnIndex;
4344
use datafusion_physical_plan::joins::utils::JoinFilter;
4445
use datafusion_physical_plan::joins::{HashJoinExec, NestedLoopJoinExec, PartitionMode};
4546
use datafusion_physical_plan::projection::ProjectionExec;
47+
use datafusion_physical_plan::sorts::sort_preserving_merge::SortPreservingMergeExec;
4648
use datafusion_physical_plan::{
4749
DisplayAs, DisplayFormatType, ExecutionPlan, PlanProperties, StatisticsArgs,
4850
StatisticsContext,
@@ -264,6 +266,84 @@ async fn test_join_with_swap() {
264266
);
265267
}
266268

269+
#[tokio::test]
270+
async fn test_join_with_swap_to_sort_preserving_merge_fetch_side() {
271+
let (big, _) = create_big_and_small();
272+
let top1_input = Arc::new(StatisticsExec::new(
273+
big_statistics(),
274+
Schema::new(vec![Field::new("top_col", DataType::Int32, false)]),
275+
));
276+
let top1 = Arc::new(
277+
SortPreservingMergeExec::new(
278+
[PhysicalSortExpr::new_default(Arc::new(Column::new(
279+
"top_col", 0,
280+
)))]
281+
.into(),
282+
top1_input,
283+
)
284+
.with_fetch(Some(1)),
285+
);
286+
287+
let join = Arc::new(
288+
HashJoinExec::try_new(
289+
Arc::clone(&big),
290+
top1,
291+
vec![(
292+
Arc::new(Column::new_with_schema("big_col", &big.schema()).unwrap()),
293+
Arc::new(Column::new("top_col", 0)),
294+
)],
295+
None,
296+
&JoinType::Inner,
297+
None,
298+
PartitionMode::Partitioned,
299+
NullEquality::NullEqualsNothing,
300+
false,
301+
)
302+
.unwrap(),
303+
);
304+
305+
let optimized_join = JoinSelection::new()
306+
.optimize(join, &ConfigOptions::new())
307+
.unwrap();
308+
let optimized_join = optimized_join
309+
.downcast_ref::<ProjectionExec>()
310+
.map(|projection| projection.input())
311+
.unwrap_or(&optimized_join);
312+
let swapped_join = optimized_join
313+
.downcast_ref::<HashJoinExec>()
314+
.expect("optimized plan should contain a hash join");
315+
316+
let left_spm = swapped_join
317+
.left()
318+
.downcast_ref::<SortPreservingMergeExec>()
319+
.expect("SPM fetch side should become the left/build input");
320+
assert_eq!(left_spm.fetch(), Some(1));
321+
let statistics_context = StatisticsContext::new();
322+
assert_eq!(
323+
statistics_context
324+
.compute(swapped_join.left().as_ref(), &StatisticsArgs::new())
325+
.unwrap()
326+
.num_rows,
327+
Precision::Inexact(1)
328+
);
329+
let left_byte_size = statistics_context
330+
.compute(swapped_join.left().as_ref(), &StatisticsArgs::new())
331+
.unwrap()
332+
.total_byte_size;
333+
let right_byte_size = big_statistics().total_byte_size;
334+
assert!(
335+
left_byte_size.get_value() < right_byte_size.get_value(),
336+
"SPM fetch side should be estimated smaller than the big side"
337+
);
338+
assert_eq!(
339+
statistics_context
340+
.compute(swapped_join.right().as_ref(), &StatisticsArgs::new())
341+
.unwrap()
342+
.num_rows,
343+
big_statistics().num_rows
344+
);
345+
}
346+
267347
#[tokio::test]
268348
async fn test_left_join_no_swap() {
269349
let (big, small) = create_big_and_small();

datafusion/physical-plan/src/sorts/sort_preserving_merge.rs

Lines changed: 63 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ use datafusion_execution::TaskContext;
3636
use datafusion_execution::memory_pool::MemoryConsumer;
3737
use datafusion_physical_expr_common::sort_expr::{LexOrdering, OrderingRequirements};
3838

39-
use crate::execution_plan::{EvaluationType, SchedulingType};
39+
use crate::execution_plan::{CardinalityEffect, EvaluationType, SchedulingType};
4040
use log::{debug, trace};
4141

4242
/// Sort preserving merge execution plan
@@ -396,7 +396,16 @@ impl ExecutionPlan for SortPreservingMergeExec {
396396
input_stats: &[Arc<Statistics>],
397397
_args: &StatisticsArgs,
398398
) -> Result<Arc<Statistics>> {
399-
Ok(Arc::clone(&input_stats[0]))
399+
let stats = input_stats[0].as_ref().clone();
400+
Ok(Arc::new(stats.with_fetch(self.fetch, 0, 1)?))
401+
}
402+
403+
fn cardinality_effect(&self) -> CardinalityEffect {
404+
if self.fetch.is_none() {
405+
CardinalityEffect::Equal
406+
} else {
407+
CardinalityEffect::LowerEqual
408+
}
400409
}
401410

402411
fn supports_limit_pushdown(&self) -> bool {
@@ -446,9 +455,12 @@ mod tests {
446455
use crate::metrics::{MetricValue, Timestamp};
447456
use crate::repartition::RepartitionExec;
448457
use crate::sorts::sort::SortExec;
458+
use crate::statistics::StatisticsContext;
449459
use crate::stream::RecordBatchReceiverStream;
450460
use crate::test::TestMemoryExec;
451-
use crate::test::exec::{BlockingExec, assert_strong_count_converges_to_zero};
461+
use crate::test::exec::{
462+
BlockingExec, StatisticsExec, assert_strong_count_converges_to_zero,
463+
};
452464
use crate::test::{self, assert_is_pending, make_partition};
453465
use crate::{collect, common};
454466

@@ -458,8 +470,9 @@ mod tests {
458470
};
459471
use arrow::compute::SortOptions;
460472
use arrow::datatypes::{DataType, Field, Schema, SchemaRef};
473+
use datafusion_common::stats::Precision;
461474
use datafusion_common::test_util::batches_to_string;
462-
use datafusion_common::{assert_batches_eq, exec_err};
475+
use datafusion_common::{ColumnStatistics, assert_batches_eq, exec_err};
463476
use datafusion_common_runtime::SpawnedTask;
464477
use datafusion_execution::RecordBatchStream;
465478
use datafusion_execution::config::SessionConfig;
@@ -524,6 +537,52 @@ mod tests {
524537
Ok(Arc::new(spm))
525538
}
526539

540+
#[test]
541+
fn test_fetch_caps_statistics() -> Result<()> {
542+
let schema = Schema::new(vec![Field::new("a", DataType::Int32, false)]);
543+
let input = Arc::new(StatisticsExec::new(
544+
Statistics {
545+
num_rows: Precision::Exact(1_000),
546+
total_byte_size: Precision::Exact(8_000),
547+
column_statistics: vec![ColumnStatistics::new_unknown()],
548+
},
549+
schema.clone(),
550+
));
551+
let sort = [PhysicalSortExpr::new_default(Arc::new(Column::new("a", 0)))].into();
552+
553+
let spm = SortPreservingMergeExec::new(sort, input).with_fetch(Some(1));
554+
let statistics =
555+
StatisticsContext::new().compute(&spm, &StatisticsArgs::new())?;
556+
557+
assert_eq!(statistics.num_rows, Precision::Exact(1));
558+
assert_eq!(statistics.total_byte_size, Precision::Inexact(8));
559+
assert!(matches!(
560+
spm.cardinality_effect(),
561+
CardinalityEffect::LowerEqual
562+
));
563+
Ok(())
564+
}
565+
566+
#[test]
567+
fn test_no_fetch_preserves_statistics() -> Result<()> {
568+
let schema = Schema::new(vec![Field::new("a", DataType::Int32, false)]);
569+
let input_stats = Statistics {
570+
num_rows: Precision::Absent,
571+
total_byte_size: Precision::Exact(8_000),
572+
column_statistics: vec![ColumnStatistics::new_unknown()],
573+
};
574+
let input = Arc::new(StatisticsExec::new(input_stats.clone(), schema.clone()));
575+
let sort = [PhysicalSortExpr::new_default(Arc::new(Column::new("a", 0)))].into();
576+
577+
let spm = SortPreservingMergeExec::new(sort, input);
578+
let statistics =
579+
StatisticsContext::new().compute(&spm, &StatisticsArgs::new())?;
580+
581+
assert_eq!(*statistics, input_stats);
582+
assert!(matches!(spm.cardinality_effect(), CardinalityEffect::Equal));
583+
Ok(())
584+
}
585+
527586
/// This test verifies that memory usage stays within limits when the tie breaker is enabled.
528587
/// Any errors here could indicate unintended changes in tie breaker logic.
529588
///

0 commit comments

Comments
 (0)