Skip to content

Apply filter early in TopK #16408

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 14 commits into from
Closed

Conversation

Dandandan
Copy link
Contributor

@Dandandan Dandandan commented Jun 14, 2025

Which issue does this PR close?

This depends on #15770

Rationale for this change

Perf changes (combined PRs - I'll rerun once #15770 is merged ):

--------------------
Benchmark clickbench_partitioned.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query        ┃        main ┃ topk-apply-filter ┃        Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 0     │    13.34 ms │          13.29 ms │     no change │
│ QQuery 1     │    18.76 ms │          18.94 ms │     no change │
│ QQuery 2     │    55.91 ms │          57.17 ms │     no change │
│ QQuery 3     │    56.22 ms │          54.68 ms │     no change │
│ QQuery 4     │   515.18 ms │         442.01 ms │ +1.17x faster │
│ QQuery 5     │   668.61 ms │         657.18 ms │     no change │
│ QQuery 6     │    14.78 ms │          14.26 ms │     no change │
│ QQuery 7     │    21.12 ms │          21.05 ms │     no change │
│ QQuery 8     │   531.48 ms │         524.48 ms │     no change │
│ QQuery 9     │   798.76 ms │         742.72 ms │ +1.08x faster │
│ QQuery 10    │   173.24 ms │         170.70 ms │     no change │
│ QQuery 11    │   185.32 ms │         187.51 ms │     no change │
│ QQuery 12    │   739.43 ms │         696.25 ms │ +1.06x faster │
│ QQuery 13    │   961.74 ms │         869.97 ms │ +1.11x faster │
│ QQuery 14    │   631.79 ms │         590.30 ms │ +1.07x faster │
│ QQuery 15    │   594.81 ms │         537.24 ms │ +1.11x faster │
│ QQuery 16    │  1344.23 ms │        1208.84 ms │ +1.11x faster │
│ QQuery 17    │  1269.51 ms │        1152.34 ms │ +1.10x faster │
│ QQuery 18    │  2341.58 ms │        2302.53 ms │     no change │
│ QQuery 19    │    50.48 ms │          45.09 ms │ +1.12x faster │
│ QQuery 20    │   960.37 ms │         973.62 ms │     no change │
│ QQuery 21    │  1046.35 ms │        1086.33 ms │     no change │
│ QQuery 22    │  1615.74 ms │        1642.84 ms │     no change │
│ QQuery 23    │  5739.41 ms │        5630.90 ms │     no change │
│ QQuery 24    │   349.55 ms │         345.82 ms │     no change │
│ QQuery 25    │   346.15 ms │         227.88 ms │ +1.52x faster │
│ QQuery 26    │   411.18 ms │         332.30 ms │ +1.24x faster │
│ QQuery 27    │  1278.73 ms │        1266.88 ms │     no change │
│ QQuery 28    │ 10645.79 ms │       10677.74 ms │     no change │
│ QQuery 29    │   434.18 ms │         438.49 ms │     no change │
│ QQuery 30    │   559.20 ms │         571.79 ms │     no change │
│ QQuery 31    │   540.42 ms │         538.21 ms │     no change │
│ QQuery 32    │  2347.65 ms │        2413.47 ms │     no change │
│ QQuery 33    │  2798.99 ms │        2721.18 ms │     no change │
│ QQuery 34    │  2972.11 ms │        2980.84 ms │     no change │
│ QQuery 35    │   807.84 ms │         827.81 ms │     no change │
│ QQuery 36    │    81.23 ms │          84.15 ms │     no change │
│ QQuery 37    │    36.27 ms │          35.63 ms │     no change │
│ QQuery 38    │    80.34 ms │          82.46 ms │     no change │
│ QQuery 39    │   130.78 ms │         133.40 ms │     no change │
│ QQuery 40    │    27.35 ms │          28.20 ms │     no change │
│ QQuery 41    │    26.51 ms │          26.34 ms │     no change │
│ QQuery 42    │    21.86 ms │          23.00 ms │  1.05x slower │
└──────────────┴─────────────┴───────────────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┓
┃ Benchmark Summary                ┃            ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━┩
│ Total Time (main)                │ 44244.29ms │
│ Total Time (topk-apply-filter)   │ 43395.86ms │
│ Average Time (main)              │  1028.94ms │
│ Average Time (topk-apply-filter) │  1009.21ms │
│ Queries Faster                   │         11 │
│ Queries Slower                   │          1 │
│ Queries with No Change           │         31 │
│ Queries with Failure             │          0 │
└──────────────────────────────────┴────────────┘


Benchmark run_topk_tpch.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query        ┃      main ┃ topk-apply-filter ┃        Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ Q1           │  28.59 ms │          16.69 ms │ +1.71x faster │
│ Q2           │  30.36 ms │          20.65 ms │ +1.47x faster │
│ Q3           │ 101.92 ms │          54.89 ms │ +1.86x faster │
│ Q4           │  27.61 ms │          22.37 ms │ +1.23x faster │
│ Q5           │  19.31 ms │          12.73 ms │ +1.52x faster │
│ Q6           │  42.45 ms │          26.39 ms │ +1.61x faster │
│ Q7           │  85.18 ms │          70.72 ms │ +1.20x faster │
│ Q8           │ 103.80 ms │          46.25 ms │ +2.24x faster │
│ Q9           │  99.54 ms │          63.53 ms │ +1.57x faster │
│ Q10          │ 141.00 ms │         100.62 ms │ +1.40x faster │
│ Q11          │  78.28 ms │          56.94 ms │ +1.37x faster │
└──────────────┴───────────┴───────────────────┴───────────────┘

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added documentation Improvements or additions to documentation physical-expr Changes to the physical-expr crates optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) common Related to common crate physical-plan Changes to the physical-plan crate labels Jun 14, 2025
@Dandandan Dandandan changed the title Apply filter in TopK Apply filter early in TopK Jun 14, 2025
@Dandandan Dandandan requested a review from adriangb June 14, 2025 13:47
@adriangb
Copy link
Contributor

Can you make a PR on our fork and we merge this into the main PR?

@adriangb
Copy link
Contributor

Or you can just push to the main PR, I gave you write access to our fork :)

My one question is: how does this optimization play with filter pushdown? If a child plan accepted the filter as Exact should we then not re-filter? A related question which you've alluded to before: if no child plan accepted the filter at all should we avoid updating it?

@Dandandan
Copy link
Contributor Author

Dandandan commented Jun 14, 2025

Or you can just push to the main PR, I gave you write access to our fork :)

My one question is: how does this optimization play with filter pushdown? If a child plan accepted the filter as Exact should we then not re-filter? A related question which you've alluded to before: if no child plan accepted the filter at all should we avoid updating it?

I think avoiding running the filter twice for "exact cases" is optimal.
In practice, I am not sure if in this case it would add much overhead: converting to rows and comparing against / updating the heap / running the compaction logic will be the most expensive part by far in this scenario.
It will be hard I think to show it being much slower somewhere.

Your second question: I think if we actually always use the filter for topk, I guess it isn't really wasteful. I think theoretically we should do it just before runing the topk instead of after (to avoid running it for the last iteration without using it). But also here I think it will be hard to show any benefit.

@Dandandan
Copy link
Contributor Author

I applied the changes to #15770 let's continue there

@Dandandan Dandandan closed this Jun 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Related to common crate core Core DataFusion crate documentation Improvements or additions to documentation optimizer Optimizer rules physical-expr Changes to the physical-expr crates 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.

Optimize TopK with filter
2 participants