sql: Add more efficient implementation of min, max, sum, avg aggregates when used as window functions.

[yuzefovich]

Adds linear-time implementations of min, max, sum, and avg
(using sliding window approach) instead of naive quadratic
version.

Addresses: #26464.

Bonus: min and max is order of magnitude faster that PG
(when window frame doesn't include the whole partition).

Release note (performance improvement): min, max, sum, avg
now take linear time when used for aggregation as window
functions.

[cockroach-teamcity]

This change is 

[yuzefovich]

@solongordon take a look if you're interested in what I've been working on :)

[nvanbenschoten]

Reviewed 2 of 4 files at r1.
Review status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/window.go, line 772 at r1 (raw file):

			frameRun.RowIdx = 0

			if frameRun.Frame != nil {

Per my comment below, I think we should explore whether this entire block can be removed.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 18 at r1 (raw file):

import (
	"context"

The spacing on these imports got a little messed up.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 33 at r1 (raw file):

	windowFunc tree.WindowFunc, evalCtx *tree.EvalContext, wfr *tree.WindowFrameRun,
) tree.WindowFunc {
	if framableAgg, ok := windowFunc.(*framableAggregateWindowFunc); ok {

Is there a reason to ever not replace the framableAggregateWindowFunc with the faster version? If not, why not do that statically and always give these optimized window functions slidingWindowFunc implementations in the tree.Overload itself?

On that note, I've gotten a little confused by AddAggregateConstructorToFramableAggregate upon re-reading it. Why don't we give the aggregate constructor to the framableAggregateWindowFunc when we build the tree.Overload in makeAggOverloadWithReturnType? It seems like all of this could be done statically.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 39 at r1 (raw file):

			min := &slidingWindowFunc{}
			min.sw = &slidingWindow{
				values: make([]indexedValue, 0, wfr.PartitionSize()),

It would be nice if we only had to use O(max window size) instead of O(partition size) space. Should be doable by turning the window into a dynamically sized ring buffer. I'd still make sure to keep it as a contiguous chunk of memory though.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 115 at r1 (raw file):

func (sw *slidingWindow) string() string {
	var buf bytes.Buffer

Use a strings.Builder. Will save an extra alloc.

---

pkg/sql/sem/builtins/window_frame_builtins_test.go, line 35 at r1 (raw file):

const maxOffset = 100

func testSlidingWindow(t *testing.T, count int) {

Nice testing!

---

Comments from Reviewable

[nvanbenschoten]

Review status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 39 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

It would be nice if we only had to use O(max window size) instead of O(partition size) space. Should be doable by turning the window into a dynamically sized ring buffer. I'd still make sure to keep it as a contiguous chunk of memory though.

That will also allow you to remove WindowFrameRun from this function, which further indicates to me that all of this can be done statically.

---

Comments from Reviewable

[yuzefovich]

Review status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/window.go, line 772 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Per my comment below, I think we should explore whether this entire block can be removed.

I added a field which indicated whether resetting behavior is required. Without it, framableAggregateWindowFunc would always reset the aggregate, even when it is not necessary which would result in quadratic performance for aggregates (that don't have a sliding window implementation) when default frame is being used. Please let me know if you see a better way.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 18 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

The spacing on these imports got a little messed up.

Nice catch, thanks.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 33 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is there a reason to ever not replace the framableAggregateWindowFunc with the faster version? If not, why not do that statically and always give these optimized window functions slidingWindowFunc implementations in the tree.Overload itself?

On that note, I've gotten a little confused by AddAggregateConstructorToFramableAggregate upon re-reading it. Why don't we give the aggregate constructor to the framableAggregateWindowFunc when we build the tree.Overload in makeAggOverloadWithReturnType? It seems like all of this could be done statically.

I think these were great suggestions, and thanks for the clarifications.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 39 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

That will also allow you to remove WindowFrameRun from this function, which further indicates to me that all of this can be done statically.

Another great idea, thanks! Not sure what the appropriate initial buffer size should be.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 115 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Use a strings.Builder. Will save an extra alloc.

Done.

---

pkg/sql/sem/builtins/window_frame_builtins_test.go, line 35 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Nice testing!

Thanks :)

---

Comments from Reviewable

[nvanbenschoten]

Reviewed 3 of 6 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/window.go, line 772 at r1 (raw file):

Previously, yuzefovich wrote…

I added a field which indicated whether resetting behavior is required. Without it, framableAggregateWindowFunc would always reset the aggregate, even when it is not necessary which would result in quadratic performance for aggregates (that don't have a sliding window implementation) when default frame is being used. Please let me know if you see a better way.

This is much better. I'd just make it clear in this comment that not resetting is an optimization.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 35 at r2 (raw file):

// ringBuffer is a deque of indexedValue's maintained over a ring buffer.
type ringBuffer struct {

It's probably worth adding some tests for this type itself.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 40 at r2 (raw file):

	tail   int // the index of the first position right after the end of the deque.

	empty bool // indicates whether the deque is empty, necessary to distinguish

Make this a useful empty value by

1. inverting this flag
2. handling cap(r.values) == 0 in ringBuffer.add and having some minimum allocation size. 8 sounds good.

Useful empty values are a general idiom in Go that end up making code a lot nicer. You then don't need to initialize this at all in makeSlidingWindow.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 63 at r2 (raw file):

	if r.len() == cap(r.values) {
		newValues := make([]*indexedValue, 2*cap(r.values))
		for pos := 0; pos < cap(r.values); pos++ {

Can we use two calls to copy here? That would be faster because it could use a memcpy directly.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 144 at r2 (raw file):

func (sw *slidingWindow) add(iv *indexedValue) {
	var newEndIdx int
	for newEndIdx = sw.values.len() - 1; newEndIdx >= 0; newEndIdx-- {

nit: Delete newEndIdx and replace it with i, which is scoped only to this loop.

for i := sw.values.len() - 1; ...

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 158 at r2 (raw file):

func (sw *slidingWindow) removeAllBefore(idx int) {
	var newStartIdx int
	for newStartIdx = 0; newStartIdx < sw.values.len() && newStartIdx < idx; newStartIdx++ {

Same thing here. No need for newStartIdx to be scoped outside of this loop. Once it's a loop index variable, might as well call it i to avoid noise.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 194 at r2 (raw file):

	w.prevEnd = end

	if w.sw.values.empty {

.len() == 0. Don't break the abstraction you fought so hard for!

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 239 at r2 (raw file):

			return w.agg.Add(ctx, tree.NewDFloat(-*v))
		case *tree.DInterval:
			return w.agg.Add(ctx, &tree.DInterval{Duration: duration.Duration{}.Sub(v.Duration)})

-v.Duration doesn't work?

[yuzefovich]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/window.go, line 772 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This is much better. I'd just make it clear in this comment that not resetting is an optimization.

Done.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 35 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

It's probably worth adding some tests for this type itself.

Done.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 40 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Make this a useful empty value by

1. inverting this flag
2. handling cap(r.values) == 0 in ringBuffer.add and having some minimum allocation size. 8 sounds good.

Useful empty values are a general idiom in Go that end up making code a lot nicer. You then don't need to initialize this at all in makeSlidingWindow.

I agree, this way the code looks nicer.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 63 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Can we use two calls to copy here? That would be faster because it could use a memcpy directly.

Yes, we can.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 144 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: Delete newEndIdx and replace it with i, which is scoped only to this loop.

for i := sw.values.len() - 1; ...

Done. At some point I was using it outside of the loop, but then changed the code and forgot the restrict the scope.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 158 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Same thing here. No need for newStartIdx to be scoped outside of this loop. Once it's a loop index variable, might as well call it i to avoid noise.

Done.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 194 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

.len() == 0. Don't break the abstraction you fought so hard for!

Nice catch, thanks.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 239 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

-v.Duration doesn't work?

As far as options I tried, it doesn't.

However, I'm not sure whether I should include changes related to Interval type since supporting this type requires more work - for instance, offsets can currently be only integers, but it could also be something like '10 days' PRECEDING. I plan to get back to it after finishing up window functions in distsql.

[nvanbenschoten]

Reviewed 1 of 3 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 239 at r2 (raw file):

As far as options I tried, it doesn't.

Oh, this is a duration.Duration, not a time.Duration. That makes sense.

However, I'm not sure whether I should include changes related to Interval type since supporting this type requires more work - for instance, offsets can currently be only integers, but it could also be something like '10 days' PRECEDING. I plan to get back to it after finishing up window functions in distsql.

Ok, please leave a TODO comment then.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 137 at r3 (raw file):

) *slidingWindow {
	return &slidingWindow{
		values:  ringBuffer{},

No need for this line.

[yuzefovich]

Thanks for your reviews!

Reviewable status: complete! 1 of 0 LGTMs obtained

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 239 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

As far as options I tried, it doesn't.

Oh, this is a duration.Duration, not a time.Duration. That makes sense.

However, I'm not sure whether I should include changes related to Interval type since supporting this type requires more work - for instance, offsets can currently be only integers, but it could also be something like '10 days' PRECEDING. I plan to get back to it after finishing up window functions in distsql.

Ok, please leave a TODO comment then.

Done.

---

pkg/sql/sem/builtins/window_frame_builtins.go, line 137 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

No need for this line.

Done.

[yuzefovich]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)