fix: reduce RANK window top-K memory from O(input) to O(K + ties) per partition - #24591
Open
jayzhan211 wants to merge 3 commits into
Open
fix: reduce RANK window top-K memory from O(input) to O(K + ties) per partition#24591jayzhan211 wants to merge 3 commits into
jayzhan211 wants to merge 3 commits into
Conversation
…rtition `WHERE rk <= K` over `RANK() OVER (PARTITION BY ... ORDER BY ...)` could fail with `Resources exhausted` on inputs the equivalent `ROW_NUMBER` query handles comfortably, on the same data and the same retained rows. Two independent problems in `PartitionedTopKRank`: 1. Each partition's heap was handed the whole input batch (`batch.clone()`), where the `ROW_NUMBER` operator gathers just that partition's rows. A batch spanning P partitions was pinned and charged P times over. With 500 partitions in one 4000-row batch and no ties at all, `rank` reported 516.1x the batch size against 14.8x for `row_number` on identical retained rows. 2. A boundary tie was stored as `(source_batch, indices)`, so one tied row pinned an entire input batch, and was charged for it, for as long as the boundary held. Retained memory grew with the input size rather than with `K + ties`. The heap now registers a per-partition gather, mirroring `PartitionedTopK`, and `TieEntry` holds a batch of only the tied rows. Adds the "no row in this group can qualify" early skip so the gather is paid only by partitions that actually contribute. Two adjacent inefficiencies in the same insert path are addressed as well. The `ROW_NUMBER` and `RANK` partition demux allocated an `OwnedRow` per input row and rebuilt a `HashMap` every batch; `DENSE_RANK` already avoided both via `entry_ref` plus a reused scratch map, and that pattern is now used by all three. `DENSE_RANK` also bucketed every row by ORDER BY value before checking admissibility; once a partition tracks its full K distinct values its boundary only ever improves, so rows above it are now skipped before they cost a bucket. Memory, one 4000-row batch over 500 partitions: `rank` 516.1x -> 18.1x the batch. `row_number` moves 14.8x -> 17.4x, which is accounting rather than bytes: the encoded partition-key buffers and the reusable scratch are now charged, where previously only the `OwnedRow` struct was counted and never its buffer. Insert throughput, 200 x 8192 rows over 256 partitions, K=10, best of two runs: `row_number` 64.5-71.9ms -> 34.6-39.5ms, `rank` 61.7-64.0ms -> 34.9-35.0ms, `dense_rank` 102.6-104.7ms -> 30.5-31.2ms. Adds two regression tests that fail before this change, and randomized differential tests for `RANK` and `DENSE_RANK` against brute-force references over 64 seeded shapes each.
…d boundary conditions
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24591 +/- ##
==========================================
- Coverage 81.38% 81.38% -0.01%
==========================================
Files 1116 1116
Lines 397960 398118 +158
Branches 397960 398118 +158
==========================================
+ Hits 323880 324007 +127
- Misses 55120 55133 +13
- Partials 18960 18978 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
The symptom
WHERE rk <= KoverRANK() OVER (PARTITION BY ... ORDER BY ...)can fail withResources exhaustedon input where the same query written withROW_NUMBERsucceeds — same data, same partitioning, and the same rows ultimately kept.
Why it happened
PartitionedTopKExecexists to replace a full sort with per-partition retention,so its entire value rests on holding only what the removed filter needed. For
RANKthat is K rows plus every row tied at the K-th ORDER BY value: a boundexpressed in rows.
PartitionedTopKRankdid not store rows. It stored references into whole inputbatches — the heap was handed
batch.clone(), and each boundary tie was kept as(source_batch, indices). A reference keeps its entire source batch alive, sowhat the operator actually retained was bounded in batches, not in rows. Both
failures below follow from that one mismatch.
Every partition in a batch charged for that whole batch. Each partition's heap
received its own clone. Those clones share buffers, so the bytes are really paid
for once — but the reservation sums the heaps independently and has no way to see
the sharing, so a batch spanning P partitions was counted P times. With 500
partitions in a single 4000-row batch and no ties at all — so
RANKretainsexactly what
ROW_NUMBERretains:Identical retained data, reported at ~35x the cost. That gap is what pushes the
query past a memory limit
ROW_NUMBERstays comfortably under, and because it isan over-count it fails without the memory ever actually being needed.
Ties held memory proportional to the input, not to K. A tie entry lives until
the boundary improves, so one tied row pinned its whole source batch for that
entire span — and ties accumulate across batches. With eight 1000-row batches each
contributing exactly one retained row, reported size grew by the full ~8 KB batch
every time, to keep eight rows. Unlike the over-count, this is memory genuinely
held, and nothing in the design bounds it by K.
Worth noting:
PartitionedTopK(ROW_NUMBER) already copied out each partition'srows instead of holding on to the input batch.
RANKwas the outlier, which isprecisely why the two diverge so sharply on identical input.
Why this PR resolves it
The fix: store the rows, not the batches they came from
Instead of keeping a reference into the input batch, the operator now copies out
the rows it actually keeps. The heap is given a copy of just that partition's
rows, and a tie entry holds a batch containing only the tied rows.
Both failures described above follow from that one substitution, so both go away
with it:
shared left to double count. Each partition's heap now owns bytes that belong to
it alone, so adding the heaps together gives the true total instead of an
over-estimate.
into the input batch any more, so it is released as soon as the operator moves
past it. What stays in memory is the kept rows themselves:
K + ties, the boundthe retention rule always promised.
Making the copy affordable
Copying is not free, and what it replaces nearly was —
batch.clone()only bumpsa reference count. Worse, most of the copying would be wasted work: once a
partition's heap is full, a group of incoming rows that are all worse than what it
already holds changes nothing at all, and with many partitions that is the normal
case rather than the exception.
So the operator checks before it copies. It compares the group's ORDER BY values
against the current cutoff — those values are already encoded for comparison, so
the check allocates nothing — and moves on if every row is worse. Without that
check,
RANKinsert takes 53.4 ms instead of 39.5 ms on the benchmark below. Itis part of the fix rather than a separate optimization.
PartitionedTopKalreadyworked this way, for the same reason.
Two related changes in the same code
Neither of the following is a memory bug, but both are the same shape of mistake:
doing work in proportion to how much data arrives rather than to how little is
kept. The first is also what makes the copying above affordable.
Grouping rows by partition key allocated memory for every single row. To sort
a batch into per-partition buckets,
ROW_NUMBERandRANKcopied each row'sencoded partition key onto the heap (
pk_rows.row(i).owned()) purely to look itup in a map — one allocation per row — then threw the map away at the end of the
batch. Only the distinct partition keys ever need storing, and there are far
fewer of those than there are rows.
DENSE_RANKalready did it that way, lookingkeys up by reference and reusing a single map for the operator's lifetime.
This is the same handful of lines the new copying sits inside, and fixing it is
what pays for that copying: with it,
RANKends up faster than before this PRinstead of trading memory for CPU.
DENSE_RANKsorted rows into buckets before deciding whether it wanted them.Every incoming row was grouped by its ORDER BY value first and only then tested
for whether it could be kept, so rows destined to be discarded still cost a bucket.
The test can come first. A partition keeps the K smallest distinct ORDER BY values
it has seen; once it has K of them, the largest is the bar a new value must beat.
That bar only ever gets stricter, because with K values held there is no free slot
for a new one — the only way in is to evict the largest and put something smaller
in its place. So a row worse than today's bar is worse than every future bar too,
and can be dropped on sight instead of being bucketed and then thrown away. Rows
exactly equal to the bar are kept, since that value is one of the K.
What changes are included in this PR?
All in
datafusion/physical-plan/src/topk/mod.rs.PartitionedTopKRank's heap is given a per-partitiontake_record_batchcopyinstead of
batch.clone();TieEntryholds a batch of only the tied rows (itsrow_indicesfield is gone, andemitno longer re-reads from the source);boundary-evicted rows are copied out one at a time. Adds the early skip that
keeps non-contributing partitions from paying for the copy.
ROW_NUMBERandRANKkey their per-partition maps on the row-encodedpartition bytes and use
entry_refagainst a reused scratch map, matchingDENSE_RANK. The encoded key bytes are now charged to the reservation; before,only the
OwnedRowstruct was counted and never its buffer.DENSE_RANKskips rows above a saturated partition's admission boundary beforebucketing them.
Are these changes tested?
Yes.
Regression tests for the two failures above — both fail on
main, pass here:test_partitioned_topk_rank_size_is_not_per_partition_batch—RANK's reportedsize stays within 2x
ROW_NUMBER's for identical retained rows.test_partitioned_topk_rank_ties_do_not_pin_input_batches— the tie list growsby rows, not by the batches those rows arrived in.
Randomized differential tests, because both changed paths turn on subtle
admission rules (
RANKmust discard its whole tie list the moment the K-th-bestvalue improves;
DENSE_RANK's new pre-filter must not swallow boundary-equalrows):
test_partitioned_topk_rank_matches_bruteforcetest_partitioned_topk_dense_rank_matches_bruteforceEach checks the operator against a brute-force
RANK/DENSE_RANK <= Kreferenceover 64 seeded random shapes, varying K, partition count, value cardinality (kept
deliberately small so ties above, at, and below the boundary are frequent), batch
count and batch size. Both were mutation-checked: removing the tie-clearing on a
boundary shift, and making the
DENSE_RANKpre-filter reject boundary-equal rows,are each caught within the first few seeds.
Existing coverage (82
topkunit tests,window_topn.slt) passes unchanged, asdoes the full extended suite:
cargo fmt --all --checkandcargo clippy --all-targets --all-features -- -D warningsare clean.
Measurements
Memory, one 4000-row batch spanning 500 partitions, no ties:
rankrow_numberrow_numberrising is accounting, not bytes: the partition-key buffers and thereusable scratch are now charged, where the old calculation counted the
OwnedRowstruct but never its buffer.
ranklanding beside it is the point: it no longercosts multiples of what
row_numbercosts for the same retained rows.Insert throughput, 200 x 8192 rows over 256 partitions, K=10, fresh random data
per batch, best of two runs:
row_numberrankdense_rankAre there any user-facing changes?
No API changes, and query results are unchanged.
WHERE rk <= KoverRANK()nowcompletes within memory limits where it could previously fail, and all three
ranking variants get faster.