exec: add a few window function operators in simple cases

[yuzefovich]

Adds support for ROW_NUMBER, RANK, and DENSE_RANK window functions
when there is a single window function in the query with no
PARTITION BY clause and with no window frame.

Addresses: #37035.

Release note: None

[cockroach-teamcity]

This change is 

[justinj]

Very cool!

Reviewed 1 of 3 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @georgeutsin, @jordanlewis, @solongordon, and @yuzefovich)

---

pkg/sql/exec/window_funcs.go, line 104 at r1 (raw file):

	r.rankIncrement = 1

	// TODO(yuzefovich): is there a better place to do this initialization?

Any function named init will be run at program initialization (you can have multiple of them):

func init() {
  ...
}

I think this code as it exists is subject to race conditions.

---

pkg/sql/exec/window_funcs.go, line 124 at r1 (raw file):

	}
	rankCol := b.ColVec(r.outputColIdx).Int64()
	if r.distinctCol != nil {

This seems like an optimization that should be happening at the planning level to me (if we have rank or dense_rank over an empty ordering we can get rid of the window function entirely and can replace it with a 1 projection).

[yuzefovich]

Any function named init will be run at program initialization (you can have multiple of them).

TIL, thanks. Also, thanks for pointing out data race condition.

This seems like an optimization that should be happening at the planning level to me (if we have rank or dense_rank over an empty ordering we can get rid of the window function entirely and can replace it with a 1 projection).

Yes, totally, agreed. I left a TODO as a reminder to remove the code once the optimizer will handle such cases.

[yuzefovich]

Guys, please take a look at this one.

[jordanlewis]

Nice work!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @georgeutsin, @jordanlewis, @solongordon, and @yuzefovich)

---

pkg/sql/exec/window_funcs.go, line 124 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

This seems like an optimization that should be happening at the planning level to me (if we have rank or dense_rank over an empty ordering we can get rid of the window function entirely and can replace it with a 1 projection).

Agreed. @yuzefovich I would just delete the special case that writes 1 everywhere - not only should the optimizer eventually do it, it's also very unimportant - someone would have to write that explicitly, and it's almost certainly not what they would have meant, so it's not important to make it fast.

---

pkg/sql/exec/window_funcs.go, line 33 at r2 (raw file):

}

var _ Operator = &rowNumberOp{}

code organization wise, I think it would be better to move all of these into a new directory. In my branch that's adding builtin implementations, I made a vecbuiltins directory. Thoughts on adding these to that? I'd also go one func per file.

---

pkg/sql/exec/window_funcs.go, line 73 at r2 (raw file):

type rankOp struct {
	input Operator
	dense bool

You mention below, but agreed this should be templated out later.

---

pkg/sql/exec/window_funcs.go, line 121 at r2 (raw file):

	}
	// TODO(yuzefovich): once optimizer can substitute RANK and DENSE_RANK that
	// have no ordering columns with `1` projection, remove this code.

You should also be able to use the new const projection code that I have for this, since it's known at planning time. I think that's probably the right move.

---

pkg/sql/exec/window_funcs.go, line 140 at r2 (raw file):

func init() {
	if atomic.CompareAndSwapInt32(&oneInt64VecInitialized, 0, 1) {

You don't need this - init gets run exactly once.

[yuzefovich]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @georgeutsin, @jordanlewis, @justinj, @solongordon, and @yuzefovich)

---

pkg/sql/exec/window_funcs.go, line 124 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Agreed. @yuzefovich I would just delete the special case that writes 1 everywhere - not only should the optimizer eventually do it, it's also very unimportant - someone would have to write that explicitly, and it's almost certainly not what they would have meant, so it's not important to make it fast.

Ok, makes sense. I removed the corresponding code.

---

pkg/sql/exec/window_funcs.go, line 33 at r2 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

code organization wise, I think it would be better to move all of these into a new directory. In my branch that's adding builtin implementations, I made a vecbuiltins directory. Thoughts on adding these to that? I'd also go one func per file.

I like these suggestions, done.

---

pkg/sql/exec/window_funcs.go, line 73 at r2 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

You mention below, but agreed this should be templated out later.

Will do the templating in the follow-up PR.

---

pkg/sql/exec/window_funcs.go, line 121 at r2 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

You should also be able to use the new const projection code that I have for this, since it's known at planning time. I think that's probably the right move.

Left a TODO.

---

pkg/sql/exec/window_funcs.go, line 140 at r2 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

You don't need this - init gets run exactly once.

Still learning :)

[yuzefovich]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @georgeutsin, @jordanlewis, @justinj, @solongordon, and @yuzefovich)

---

pkg/sql/exec/vecbuiltins/row_number.go, line 60 at r3 (raw file):

	rowNumberCol := b.ColVec(r.outputColIdx).Int64()
	sel := b.Selection()
	// TODO(yuzefovich): template selection out.

I'm not sure whether templating selection vectors out is worth. Is it? I don't think it'll be cleaner or more performant.

[yuzefovich]

Friendly ping @jordanlewis @asubiotto.

[jordanlewis]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @asubiotto, @jordanlewis, @justinj, @solongordon, and @yuzefovich)

---

pkg/sql/exec/vecbuiltins/rank.go, line 58 at r3 (raw file):

) (exec.Operator, error) {
	if len(orderingCols) == 0 {
		// TODO(yuzefovich): add const projection in this case once that's merged.

I think that is merged already.

---

pkg/sql/exec/vecbuiltins/row_number.go, line 60 at r3 (raw file):

Previously, yuzefovich wrote…

I'm not sure whether templating selection vectors out is worth. Is it? I don't think it'll be cleaner or more performant.

Definitely not more performant, and probably somewhat less clean. probably not too important to template this.

[yuzefovich]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @asubiotto, @jordanlewis, @justinj, @solongordon, and @yuzefovich)

---

pkg/sql/exec/vecbuiltins/rank.go, line 58 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I think that is merged already.

Indeed, thanks.

---

pkg/sql/exec/vecbuiltins/row_number.go, line 60 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Definitely not more performant, and probably somewhat less clean. probably not too important to template this.

Agreed, thanks.

[yuzefovich]

TFTR!

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)