Linear search support for Window Group queries #5286
Conversation
…tions and comment clarifications
github-actions
bot
added
core
There was a problem hiding this comment.
This PR removes the old GROUPS mechanism and replaces it with the amortized linear search. This reduces code size and yields performance gains simultaneously.
The best kind of changes 😍
I went through this PR fairly carefully -- it all looks like great work, as always.
Thanks @mustafasrepo
| Ok(window_frame) | ||
| } | ||
| let window_frame = window_frame.clone().try_into()?; | ||
| regularize(window_frame, order_by.len()) |
There was a problem hiding this comment.
I don't understand why regularize is called in the serialization code. It makes sense to be called as part of the SQL planner (or LogicalPlanBuilder) so that by the time the structures were serialized / deserialized they should already be valid (and if they are not it should be an error).
There was a problem hiding this comment.
Agreed. That logic was already here from before (in addition to being elsewhere too). In this PR we refactored it out into the regularize function as a first step, so at least there is no code duplication anymore. This way, we will be able to remove it easily later on.
|
Argh, using GH to incorporate the suggestions resulted in a formatting error, just sent a manual commit again. Feel free to merge after CI passes. |
|
Benchmark runs are scheduled for baseline = ded897e and contender = 79ecf94. 79ecf94 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
* add naive linear search * Add last range to decrease search size * minor changes * add low, high arguments * Go back to old API, improve comments, refactors * Linear Groups implementation * Resolve linter errors * remove old unit tests * simplifications * Add unit tests * Remove sort options from GROUPS calculations, various code simplifications and comment clarifications * New TODOs to fix * Address reviews * Fix error * Prehandle range current row and unbounded following case * Fix error * Move a check from execution to planning, reduce code duplication * Incorporate review suggestion (with cargo fmt fix) --------- Co-authored-by: Mehmet Ozan Kabak <[email protected]>
Which issue does this PR close?
Closes 5213.
Closes 5275.
Rationale for this change
This is a continuation of #4989, it extends amortized linear search support to window frames in GROUPS mode. As discussed in the preceding PR, this results in significant performance gains, which is the main purpose of this PR.
What changes are included in this PR?
This PR removes the old GROUPS mechanism and replaces it with the amortized linear search. This reduces code size and yields performance gains simultaneously.
Are there any user-facing changes?
No.