Skip to content

perf: Extend WindowTopN to dense_rank#23869

Open
SubhamSinghal wants to merge 2 commits into
apache:mainfrom
SubhamSinghal:window-topn-dense-rank
Open

perf: Extend WindowTopN to dense_rank#23869
SubhamSinghal wants to merge 2 commits into
apache:mainfrom
SubhamSinghal:window-topn-dense-rank

Conversation

@SubhamSinghal

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Naive plan for Filter(dr <= K) → BoundedWindowAggExec(DENSE_RANK) → SortExec(pk, ob) → input sorts the full input even though only rows at the K distinct-smallest ob values per partition are needed. DENSE_RANK is stable under tail-pruning, so the rewrite preserves ranks for every retained row.

What changes are included in this PR?

Are these changes tested?

Yes

h2o bench — Q24–Q29 added; top-2 on 10M-row large, 3-iter avg vs enable_window_topn=false:

# Cardinality OFF (ms) ON (ms) Δ
Q24 ~100 parts 308.19 114.45 2.20× faster
Q25 ~1K, low ties 261.24 113.85 2.13× faster
Q26 ~1K, heavy ties 296.54 124.42 2.09× faster
Q27 ~10K, low ties 250.83 131.85 1.70× faster
Q28 ~10K, heavy ties 281.00 149.00 1.76× faster
Q29 ~100K parts 264.32 304.37 1.40× slower

Are there any user-facing changes?

No breaking changes. Rule fires on DENSE_RANK when datafusion.optimizer.enable_window_topn = true; flag stays
false by default

@github-actions github-actions Bot added optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Jul 24, 2026
@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details
     Cloning apache/main
    Building datafusion v54.1.0 (current)
       Built [ 111.339s] (current)
     Parsing datafusion v54.1.0 (current)
      Parsed [   0.039s] (current)
    Building datafusion v54.1.0 (baseline)
       Built [ 109.622s] (baseline)
     Parsing datafusion v54.1.0 (baseline)
      Parsed [   0.038s] (baseline)
    Checking datafusion v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.855s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [ 224.494s] datafusion
    Building datafusion-physical-optimizer v54.1.0 (current)
       Built [  41.374s] (current)
     Parsing datafusion-physical-optimizer v54.1.0 (current)
      Parsed [   0.025s] (current)
    Building datafusion-physical-optimizer v54.1.0 (baseline)
       Built [  41.183s] (baseline)
     Parsing datafusion-physical-optimizer v54.1.0 (baseline)
      Parsed [   0.024s] (baseline)
    Checking datafusion-physical-optimizer v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.157s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [  83.850s] datafusion-physical-optimizer
    Building datafusion-physical-plan v54.1.0 (current)
       Built [  37.773s] (current)
     Parsing datafusion-physical-plan v54.1.0 (current)
      Parsed [   0.156s] (current)
    Building datafusion-physical-plan v54.1.0 (baseline)
       Built [  38.223s] (baseline)
     Parsing datafusion-physical-plan v54.1.0 (baseline)
      Parsed [   0.154s] (baseline)
    Checking datafusion-physical-plan v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.918s] 223 checks: 222 pass, 1 fail, 0 warn, 30 skip

--- failure enum_variant_added: enum variant added on exhaustive enum ---

Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/enum_variant_added.ron

Failed in:
  variant WindowFnKind:DenseRank in /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/sorts/partitioned_topk.rs:76

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  78.587s] datafusion-physical-plan
    Building datafusion-sqllogictest v54.1.0 (current)
       Built [ 193.470s] (current)
     Parsing datafusion-sqllogictest v54.1.0 (current)
      Parsed [   0.023s] (current)
    Building datafusion-sqllogictest v54.1.0 (baseline)
       Built [ 191.859s] (baseline)
     Parsing datafusion-sqllogictest v54.1.0 (baseline)
      Parsed [   0.025s] (baseline)
    Checking datafusion-sqllogictest v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.114s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [ 388.349s] datafusion-sqllogictest

@github-actions github-actions Bot added the auto detected api change Auto detected API change label Jul 24, 2026
@codecov-commenter

codecov-commenter commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.94727% with 64 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.72%. Comparing base (582453b) to head (32b3b5c).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/physical-plan/src/topk/mod.rs 87.84% 13 Missing and 49 partials ⚠️
...fusion/physical-plan/src/sorts/partitioned_topk.rs 87.50% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #23869    +/-   ##
========================================
  Coverage   80.72%   80.72%            
========================================
  Files        1090     1090            
  Lines      370384   370914   +530     
  Branches   370384   370914   +530     
========================================
+ Hits       298975   299414   +439     
- Misses      53590    53623    +33     
- Partials    17819    17877    +58     

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

@comphead comphead changed the title perf: Extend WindowTopN to dense rank perf: Extend WindowTopN to dense_rank Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto detected api change Auto detected API change core Core DataFusion crate optimizer Optimizer rules physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants