Skip to content

Commit 2eb9b29

Browse files
committed
feat(physical-optimizer): support FilterExec with embedded projection in WindowTopN
The WindowTopN rule bailed out unconditionally when the FilterExec at the top of the pattern carried an embedded projection (from an earlier filter/projection pushdown pass), even when the underlying pattern otherwise matched. That skipped rewrite path caused ROW_NUMBER top-K-per-group queries to fall back to a full sort in production. Capture the FilterExec's projection indices at the start, run the existing PartitionedTopKExec rewrite as usual, and re-apply the captured projection as an outer ProjectionExec so the transformed plan preserves the original output schema. Adds a regression test in datafusion/core/tests/physical_optimizer/window_topn.rs covering the FilterExec-with-projection shape.
1 parent 7f6cc60 commit 2eb9b29

2 files changed

Lines changed: 106 additions & 9 deletions

File tree

datafusion/core/tests/physical_optimizer/window_topn.rs

Lines changed: 67 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ use datafusion_physical_expr_common::sort_expr::{LexOrdering, PhysicalSortExpr};
3333
use datafusion_physical_optimizer::PhysicalOptimizerRule;
3434
use datafusion_physical_optimizer::window_topn::WindowTopN;
3535
use datafusion_physical_plan::displayable;
36-
use datafusion_physical_plan::filter::FilterExec;
36+
use datafusion_physical_plan::filter::{FilterExec, FilterExecBuilder};
3737
use datafusion_physical_plan::placeholder_row::PlaceholderRowExec;
3838
use datafusion_physical_plan::projection::ProjectionExec;
3939
use datafusion_physical_plan::sorts::sort::SortExec;
@@ -607,3 +607,69 @@ fn dense_rank_no_change() -> Result<()> {
607607
);
608608
Ok(())
609609
}
610+
611+
/// Regression: FilterExec that carries an embedded projection (e.g. from
612+
/// an earlier filter/projection pushdown pass) used to make the rule bail
613+
/// out entirely. The rule now captures the projection, applies the
614+
/// PartitionedTopKExec rewrite, and wraps the result in a ProjectionExec
615+
/// that reproduces the original output schema.
616+
#[test]
617+
fn filter_with_projection_still_rewrites() -> Result<()> {
618+
let s = schema();
619+
let input: Arc<dyn ExecutionPlan> = Arc::new(PlaceholderRowExec::new(Arc::clone(&s)));
620+
621+
let ordering = LexOrdering::new(vec![
622+
PhysicalSortExpr::new_default(col("pk", &s)?).asc(),
623+
PhysicalSortExpr::new_default(col("val", &s)?).asc(),
624+
])
625+
.unwrap();
626+
let sort: Arc<dyn ExecutionPlan> =
627+
Arc::new(SortExec::new(ordering, input).with_preserve_partitioning(true));
628+
629+
let partition_by = vec![col("pk", &s)?];
630+
let order_by = vec![PhysicalSortExpr::new_default(col("val", &s)?).asc()];
631+
let window_expr = Arc::new(StandardWindowExpr::new(
632+
create_udwf_window_expr(
633+
&row_number_udwf(),
634+
&[],
635+
&s,
636+
"row_number".to_string(),
637+
false,
638+
)?,
639+
&partition_by,
640+
&order_by,
641+
Arc::new(WindowFrame::new_bounds(
642+
WindowFrameUnits::Rows,
643+
WindowFrameBound::Preceding(ScalarValue::UInt64(None)),
644+
WindowFrameBound::CurrentRow,
645+
)),
646+
));
647+
let window: Arc<dyn ExecutionPlan> = Arc::new(BoundedWindowAggExec::try_new(
648+
vec![window_expr],
649+
sort,
650+
InputOrderMode::Sorted,
651+
true,
652+
)?);
653+
654+
// Filter: row_number@2 <= 3, with an embedded projection that keeps
655+
// only [pk, val] (drops the row_number column) — the shape produced
656+
// by filter/projection pushdown when downstream doesn't need the
657+
// window column.
658+
let rn_col = Arc::new(Column::new("row_number", 2));
659+
let limit_lit = lit(ScalarValue::UInt64(Some(3)));
660+
let predicate = Arc::new(BinaryExpr::new(rn_col, Operator::LtEq, limit_lit));
661+
let filter: Arc<dyn ExecutionPlan> = Arc::new(
662+
FilterExecBuilder::new(predicate, window)
663+
.apply_projection(Some(vec![0, 1]))?
664+
.build()?,
665+
);
666+
667+
let optimized = optimize(filter)?;
668+
assert_snapshot!(plan_str(optimized.as_ref()), @r#"
669+
ProjectionExec: expr=[pk@0 as pk, val@1 as val]
670+
BoundedWindowAggExec: wdw=[row_number: Field { "row_number": UInt64 }, frame: ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW], mode=[Sorted]
671+
PartitionedTopKExec: fn=row_number, fetch=3, partition=[pk@0], order=[val@1 ASC]
672+
PlaceholderRowExec
673+
"#);
674+
Ok(())
675+
}

datafusion/physical-optimizer/src/window_topn.rs

Lines changed: 39 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ use datafusion_common::config::ConfigOptions;
5656
use datafusion_common::tree_node::{Transformed, TransformedResult, TreeNode};
5757
use datafusion_common::{Result, ScalarValue};
5858
use datafusion_expr::Operator;
59+
use datafusion_physical_expr::PhysicalExpr;
5960
use datafusion_physical_expr::expressions::{BinaryExpr, Column, Literal};
6061
use datafusion_physical_expr::window::StandardWindowExpr;
6162
use datafusion_physical_plan::ExecutionPlan;
@@ -132,10 +133,14 @@ impl WindowTopN {
132133
// Step 1: Match FilterExec at the top
133134
let filter = plan.downcast_ref::<FilterExec>()?;
134135

135-
// Don't handle filters with projections
136-
if filter.projection().is_some() {
137-
return None;
138-
}
136+
// A projection embedded in the FilterExec (from an earlier
137+
// filter/projection pushdown pass) is captured here and re-applied
138+
// via a wrapping ProjectionExec at the end so the rewrite preserves
139+
// the original output schema.
140+
let filter_projection: Option<Vec<usize>> = filter
141+
.projection()
142+
.as_ref()
143+
.map(|p| p.iter().copied().collect());
139144

140145
// Step 2: Extract limit from predicate (rn <= K, rn < K, etc.)
141146
let (col_idx, limit_n) = extract_window_limit(filter.predicate())?;
@@ -195,13 +200,41 @@ impl WindowTopN {
195200
.ok()?;
196201

197202
// Step 9: If ProjectionExec was between Filter and Window, rebuild it
198-
let result = match proj_between {
203+
let mut result = match proj_between {
199204
Some(proj) => Arc::clone(&child_as_arc(proj))
200205
.with_new_children(vec![new_window])
201206
.ok()?,
202207
None => new_window,
203208
};
204209

210+
// Step 10: Re-apply the FilterExec's embedded projection (if any)
211+
// as an outer ProjectionExec. The projection indices refer to
212+
// columns in `filter.input().schema()`, which equals `result`'s
213+
// schema at this point (Steps 8-9 preserve schema), so the
214+
// indices remain valid.
215+
if let Some(indices) = filter_projection {
216+
let input_schema = result.schema();
217+
let field_count = input_schema.fields().len();
218+
// Validate before indexing: an out-of-range index would panic
219+
// in `input_schema.field(idx)`. Bail out of the rewrite instead
220+
// so a malformed FilterExec projection can never crash the
221+
// optimizer.
222+
if indices.iter().any(|&idx| idx >= field_count) {
223+
return None;
224+
}
225+
let projection_exprs: Vec<(Arc<dyn PhysicalExpr>, String)> = indices
226+
.iter()
227+
.map(|&idx| {
228+
let field = input_schema.field(idx);
229+
(
230+
Arc::new(Column::new(field.name(), idx)) as Arc<dyn PhysicalExpr>,
231+
field.name().clone(),
232+
)
233+
})
234+
.collect();
235+
result = Arc::new(ProjectionExec::try_new(projection_exprs, result).ok()?);
236+
}
237+
205238
Some(result)
206239
}
207240
}
@@ -264,9 +297,7 @@ impl PhysicalOptimizerRule for WindowTopN {
264297
/// - `10 >= rn` → `Some((2, 10))`
265298
/// - `rn = 1` → `None` (equality not supported)
266299
/// - `val <= 5` → `Some((1, 5))` (caller must verify it's a window column)
267-
fn extract_window_limit(
268-
predicate: &Arc<dyn datafusion_physical_expr::PhysicalExpr>,
269-
) -> Option<(usize, usize)> {
300+
fn extract_window_limit(predicate: &Arc<dyn PhysicalExpr>) -> Option<(usize, usize)> {
270301
let binary = predicate.downcast_ref::<BinaryExpr>()?;
271302
let op = binary.op();
272303
let left = binary.left();

0 commit comments

Comments
 (0)