Repository navigation
perf: Partitioned topk emit row number - #26133
SubhamSinghal wants to merge 4 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #26133 +/- ##
==========================================
+ Coverage 82.73% 82.81% +0.08%
==========================================
Files 1147 1149 +2
Lines 449389 452653 +3264
Branches 449389 452653 +3264
==========================================
+ Hits 371788 374853 +3065
- Misses 54943 54959 +16
- Partials 22658 22841 +183 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@kosiew @jayzhan-synnada @kumarUjjawal Can you help in reviewing this PR |
|
benchmark run shows improved performance. |
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The change removes the redundant window operator and adds good coverage for ranking values and ordering.
I found one correctness issue with custom window functions that share a name with the built-in ranking functions. The optimizer can remove their evaluator and silently produce different results. Please address this and add regression tests before merging.
| // operator appends a single column, and it has to reproduce the whole | ||
| // of the window node's output schema. With two or more the window node | ||
| // stays and computes them as before. | ||
| let ranking_field = if window_exprs.len() == 1 { |
There was a problem hiding this comment.
This removes the window evaluator based only on the function name, but a custom UDWF named row_number, rank, or dense_rank can have different behavior. I reproduced a custom row_number returning constant 7 that produces [1, 2] instead of [7, 7] with WindowTopN enabled, even when no rows are pruned. Please check the actual WindowUDF identity in supported_window_fn, using equality with the built-in functions rather than checking the Rank implementation type, which is shared by several functions. Please also add regression tests for same-name custom functions returning UInt64 and another type, and ideally a custom sibling expression.
Which issue does this PR close?
Rationale for this change
With
enable_window_topn,WindowTopNrewritesFilter(rn <= K) → BoundedWindowAggExec → SortExecinto:The window node stays only to produce
rn, but the operator already knows every retained row's rank when it emits it: rows leave in(partition keys, order keys)order, and the retained set is exactly the rows with rank ≤ K. The node's cost scales with the operator's output (K × partitions) and never amortizes, because every batch it sees is dense in partition boundaries.What changes are included in this PR?
PartitionedTopKExec::try_newtakesranking_field: Option<FieldRef>. When set, the operator appends aUInt64column with each row's rank, andWindowTopNdrops the window node:Each policy derives the rank from what it already holds.
ROW_NUMBER: position in the partition's drain.RANK: position of the first row with the same key; boundary ties take the boundary key's.DENSE_RANK: index of the distinct-key group. All three rely on the retained set being a complete order-prefix of its partition.Applied only when the window node has exactly one expression, since the operator appends one column. Otherwise the node is kept, as today. The output schema is the node's (input fields plus that field), so column indices above are unchanged and
schema_check()holds.compute_propertiesdeclares the ordering the removed node published,[partition keys..., rank ASC NULLS LAST], soORDER BY pk, rnstays sort-free.Are these changes tested?
topk/mod.rs: the rank column for each policy, including boundary ties, an eviction that moves a row to the tie list, aDENSE_RANKgroup gathered from several input batches, and output spanning severalbatch_sizechunks.sorts/partitioned_topk.rs: the widened schema, and the[pk, rn]ordering (but not[rn]alone).core/tests/physical_optimizer/window_topn.rs: the node is removed for one expression and kept for two;ORDER BY pk, rnplans noSortExecfor all three functions; end to end, the emitted column equals the flag-off result (ties, 4 partitions, batch size 4).window_topn.sltandrange_partitioning.slt: EXPLAIN output updated, query results unchanged.Benchmark
h2o
window.sqlk=2 sweeps onJ1_1e7_1e7_NA.parquet(10 M rows, 14 partitions),enable_window_topn = truein both arms.ROW_NUMBERRANKDENSE_RANKOpen question for reviewers
WindowTopNrecognises ranking functions by name. Now that the window node is removed, a user-registeredrank/row_number/dense_ranknever runs its own evaluator:UInt64, the query fails at execution (column types must match schema types);UInt64, its values are silently replaced by the built-in's.Which fix do you prefer?
== rank_udwf()etc. (WindowUDFequality compares the concrete type and its fields). This needsdatafusion-functions-windowas a normal dependency ofdatafusion-physical-optimizer, which adds no crates for anyone already usingdatafusion.WindowUDFImplmethod declaring top-K ranking semantics, likelimit_effect. It is a public API addition, so probably a follow-up.UInt64. This fixes the error but not the silent replacement.Are there any user-facing changes?
Only with
enable_window_topn(default false):EXPLAINno longer shows the window node for single-expression ranking queries, andPartitionedTopKExecshowsemit=[...]. Query results are unchanged.PartitionedTopKExec::try_newgains a parameter, so this is a breaking API change; please add theapi changelabel.PartitionedTopKExec::ranking_field()is the new accessor.