Skip to content

fix: PartitionedTopKRank counts tie rows twice in output_rows - #25535

Open
jayzhan211 wants to merge 1 commit into
apache:mainfrom
jayzhan211:fix/topk-rank-output-rows
Open

jayzhan211 wants to merge 1 commit into
apache:mainfrom
jayzhan211:fix/topk-rank-output-rows

Conversation

@jayzhan211

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

  • N/A — no issue filed; found while reading the code.

Rationale for this change

For a RANK() window Top-N (PartitionedTopKExec: fn=rank), the output_rows and output_batches metrics shown by EXPLAIN ANALYZE are too high whenever rows tie at the K-th value.

PartitionedTopKRank::emit calls record_output on every tie batch before pushing it into the BatchCoalescer, and then calls record_output again on each completed batch coming out of that coalescer — which already contains those tie rows. Heap rows are only counted at the second site, so each tie row is counted twice and each tie batch adds a phantom output batch.

Example: K = 2, one partition with values 5, 5, 10, 5. Three rows are emitted in one batch, but the metrics report output_rows=4, output_batches=2.

PartitionedTopK (ROW_NUMBER) and PartitionedTopKDenseRank only record at the coalescer output and are not affected.

What changes are included in this PR?

Remove the extra record_output call on tie batches so every emitted row is counted once, at the coalescer output, as in the other two operators. Query results are unchanged; only the metrics are corrected.

What is the testing strategy for this PR?

New unit test test_partitioned_topk_rank_output_rows_counts_ties_once, which emits heap rows plus a boundary tie and asserts output_rows / output_batches equal what the stream actually produced. It fails on main with (2, 4) vs (1, 3).

Are there any user-facing changes?

output_rows / output_batches reported for PartitionedTopKExec with fn=rank are now accurate when ties are present. No API changes.

@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Sep 20, 2026
@jayzhan211
jayzhan211 marked this pull request as ready for review September 20, 2026 09:03
@jayzhan211
jayzhan211 requested review from alamb and kosiew September 20, 2026 09:03
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.86207% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.38%. Comparing base (d20936c) to head (de09948).

Files with missing lines Patch % Lines
datafusion/physical-plan/src/topk/mod.rs 75.86% 1 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25535      +/-   ##
==========================================
- Coverage   82.38%   82.38%   -0.01%     
==========================================
  Files        1138     1138              
  Lines      434491   434519      +28     
  Branches   434491   434519      +28     
==========================================
+ Hits       357969   357981      +12     
- Misses      54875    54884       +9     
- Partials    21647    21654       +7     

☔ 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.

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.

2 participants