sql: speed up aggregates as window functions in some cases #38836
Conversation
|
This change is |
|
This should be an enormous speedup for aggregations with ORDER BY since those are planned as window functions internally with |
There was a problem hiding this comment.
Nice! This is huge.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis, @justinj, @ridwanmsharif, and @yuzefovich)
pkg/sql/sem/tree/window_funcs.go, line 525 at r1 (raw file):
precedingConfirmed := wfr.Frame.Bounds.StartBound.BoundType == UnboundedPreceding followingConfirmed := wfr.Frame.Bounds.EndBound.BoundType == UnboundedFollowing if wfr.Frame.Mode == ROWS || wfr.Frame.Mode == GROUPS {
this section feels a little overly clever to me :) not convinced this case is that important compared to the other one, but your call.
There was a problem hiding this comment.
I can't tell by the logic test but is the more complex case (groups and rows) also tested?
Reviewable status:
There was a problem hiding this comment.
Yes, the newly added logic test checks all combinations (I've just confirmed it by step-by-step debugging).
Reviewable status:
pkg/sql/sem/tree/window_funcs.go, line 525 at r1 (raw file):
Previously, justinj (Justin Jaffray) wrote…
this section feels a little overly clever to me :) not convinced this case is that important compared to the other one, but your call.
I agree that this case is less important, yet it can be hit even with small offsets (when partitions are small). I wouldn't say it is too clever - we're simply checking the boundary conditions, and if those are satisfied, then "internal" conditions will also be satisfied. Maybe my comments are confusing?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/window_funcs.go, line 525 at r1 (raw file):
Previously, yuzefovich wrote…
I agree that this case is less important, yet it can be hit even with small offsets (when partitions are small). I wouldn't say it is too clever - we're simply checking the boundary conditions, and if those are satisfied, then "internal" conditions will also be satisfied. Maybe my comments are confusing?
No, the comments are fine, I just think it's proportionally a lot of stuff to understand for a case I'm not convinced is common. I'm not bothered too much either way.
There was a problem hiding this comment.
TFTRs!
bors r+
Reviewable status:
pkg/sql/sem/tree/window_funcs.go, line 525 at r1 (raw file):
Previously, justinj (Justin Jaffray) wrote…
No, the comments are fine, I just think it's proportionally a lot of stuff to understand for a case I'm not convinced is common. I'm not bothered too much either way.
Ok, I'll keep this case then.
|
bors r- |
Canceled |
In a case when an aggregate is computed as a window function over a window frame such that all rows of the partitions are inside of the window for each of the row, the result of aggregation is the same for all rows. Previously, we would recompute it every time, but now we're computing it only once. Release note: None
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/window_funcs.go, line 525 at r1 (raw file):
Previously, yuzefovich wrote…
Ok, I'll keep this case then.
I've just realized that my comments, indeed, were confusing.
|
bors r+ |
38836: sql: speed up aggregates as window functions in some cases r=yuzefovich a=yuzefovich In a case when an aggregate is computed as a window function over a window frame such that all rows of the partitions are inside of the window for each of the row, the result of aggregation is the same for all rows. Previously, we would recompute it every time, but now we're computing it only once. Addresses: #38818. Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]>
|
What kind of speed ups are we talking? Also, nice rapid fixing! |
|
So the speedup is going from quadratic behavior to linear. For concrete numbers, the query |
Build succeeded |
In a case when an aggregate is computed as a window function over
a window frame such that all rows of the partitions are inside of
the window for each of the row, the result of aggregation is the
same for all rows. Previously, we would recompute it every time,
but now we're computing it only once.
Addresses: #38818.
Release note: None