Skip to content

perf: avoid capturing unused evicted rows in TopK - #25599

Closed
lyne7-sc wants to merge 2 commits into
apache:mainfrom
lyne7-sc:perf/topk-eviction
Closed

lyne7-sc wants to merge 2 commits into
apache:mainfrom
lyne7-sc:perf/topk-eviction

Conversation

@lyne7-sc

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

  • Closes #.

Rationale for this change

TopK queries with frequent heap replacements spend time capturing evicted rows that ordinary TopK and partitioned ROW_NUMBER TopK immediately discard. Avoiding this work reduces allocation, encoded-key copying, and RecordBatch column-reference cloning.

What changes are included in this PR?

  • Split heap insertion into add() and add_with_eviction(), sharing heap maintenance through a const-generic implementation.
  • Skip eviction capture for ordinary TopK and partitioned ROW_NUMBER TopK.
  • Preserve eviction capture for RANK, which needs evicted rows to retain boundary ties.

What is the testing strategy for this PR?

  • Add test_topk_heap_same_and_cross_batch_eviction to cover both capture and discard modes, checking evicted values, encoded keys, batch release, and final output.
  • Existing SLT coverage in topk.slt and window_topn.slt verifies ordinary TopK and partitioned ROW_NUMBER/RANK behavior.

Are there any user-facing changes?

This is an internal performance improvement. Query results and public APIs are unchanged.

Benchmarks

  group                                               before                                 new
  -----                                               ------                                 ---
  topk_replacements/u64_narrow_k128/ascending         1.01     58.0±3.93µs        ? ?/sec    1.00     57.5±0.80µs        ? ?/sec
  topk_replacements/u64_narrow_k128/descending        1.49      7.9±0.04ms        ? ?/sec    1.00      5.3±0.20ms        ? ?/sec
  topk_replacements/u64_narrow_k128/random            1.10    235.3±3.58µs        ? ?/sec    1.00    213.1±4.72µs        ? ?/sec
  topk_replacements/utf8_64_narrow_k128/ascending     1.05   406.2±63.66µs        ? ?/sec    1.00   387.7±13.97µs        ? ?/sec
  topk_replacements/utf8_64_narrow_k128/descending    1.36     12.7±0.20ms        ? ?/sec    1.00      9.4±0.42ms        ? ?/sec
  topk_replacements/utf8_64_narrow_k128/random        1.01    667.4±8.85µs        ? ?/sec    1.00   659.2±34.78µs        ? ?/sec
  topk_replacements/utf8_64_wide_k1024/ascending      1.00   556.5±17.64µs        ? ?/sec    1.01   559.9±21.18µs        ? ?/sec
  topk_replacements/utf8_64_wide_k1024/descending     1.44     23.7±0.45ms        ? ?/sec    1.00     16.4±2.02ms        ? ?/sec
  topk_replacements/utf8_64_wide_k1024/random         1.12      3.8±0.07ms        ? ?/sec    1.00      3.4±0.07ms        ? ?/sec
  topk_replacements/utf8_64_wide_k128/ascending       1.02        397.62µs        ? ?/sec    1.00        390.92µs        ? ?/sec
  topk_replacements/utf8_64_wide_k128/descending      1.96     18.1±0.31ms        ? ?/sec    1.00      9.2±0.05ms        ? ?/sec
  topk_replacements/utf8_64_wide_k128/random          1.11   806.7±22.68µs        ? ?/sec    1.00   724.9±13.62µs        ? ?/sec

@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Sep 22, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.42%. Comparing base (6574a8c) to head (0cd6760).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25599      +/-   ##
==========================================
- Coverage   82.42%   82.42%   -0.01%     
==========================================
  Files        1138     1139       +1     
  Lines      435501   435483      -18     
  Branches   435501   435483      -18     
==========================================
- Hits       358955   358927      -28     
- Misses      54841    54843       +2     
- Partials    21705    21713       +8     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kosiew

kosiew commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

@lyne7-sc
I would love to review this after you resolve the merge conflicts.

@lyne7-sc

Copy link
Copy Markdown
Contributor Author

Thanks @kosiew for taking a look!

After checking the conflicts, I found that #25968 already eliminates the unnecessary eviction capture this PR targeted, so I'll close this PR.

@kosiew
kosiew marked this pull request as draft October 15, 2026 09:09
@lyne7-sc lyne7-sc closed this Oct 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants