sql: Add initial support for window functions

[nvanbenschoten]

Closes #2475.

This change adds support for a subset of all window function
functionality. Window functions can be run over either pre-existing
aggregate builtins or a set of window function-only builtins. The change
only supports running window functions over aggregation functions for now.
It fully supports the PARTITION BY and ORDER BY clauses, but does not
yet support frame clause for window definitions.

Future related work:

• Support window frame clause
• Add built-in window functions
• Improve partitioning/sorting/aggregating efficiency (see referenced
  papers in TODOs)
• Improve inter- and intra-partition parallelism
• Avoid adding unnecessary extra renders to selectNode
• Support WITHIN GROUP and FILTER clauses

---

This change is

[maddyblue]

Review status: 0 of 23 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

sql/window.go, line 465 [r1] (raw file):

}

// populateValues populated n.values with final datum values after computing

populated -> populates

---

sql/parser/sql.y, line 3740 [r1] (raw file):

  WINDOW window_definition_list
  {
    $$.val = $2.window();

Unnecessary semicolon

---

Comments from Reviewable

[knz]

A couple general remarks:

• this looks nice and I absolutely love that we can support this; however
• window functions are notorious for requiring much storage to buffer the partitions. I question the general idea to do this work before either sql: Add support for TEMP tables #5807 with disk-based storage or dist-SQL lands, this will blow up much faster than a join and we can't optimize windowing with smart index orderings.
• because of this, please hold off until sql: preliminary mechanism to track and limit SQL memory usage. #8691 lands, or even better either help me land it faster with helping with reviewing or start rebasing on top of it already. I wouldn't feel confortable modifying your windowing code since I don't understand it fully (yet) whereas you probably will understand my code, which is rather straightforward, much faster.

---

Reviewed 23 of 23 files at r1.
Review status: all files reviewed at latest revision, 21 unresolved discussions, some commit checks failed.

---

sql/window.go, line 30 [r1] (raw file):

  "github.com/pkg/errors"
)

Please explain in a comment here your terminology, in particular the difference between:

• window specification
• window definition
• window function
• window function application
• window node
  For each specify where they start to exist in the lifetime of a query and what part of the code is responsible for creating them.

---

sql/window.go, line 111 [r1] (raw file):

      windowDef := *windowFn.expr.WindowDef

      // Override referenced window specifications if applicable.

Explain why / when overriding is necessary.

---

sql/window.go, line 114 [r1] (raw file):

      modifyRef := false
      var refName string
      switch {

I think for readability this switch and the test on refName below could be moved to their own function.

---

sql/window.go, line 118 [r1] (raw file):

          // SELECT rank() OVER (w) FROM t WINDOW w as (...)
          // We copy the referenced window specification, and modify it if necessary.
          refName = strings.ToLower(windowDef.RefName)

Why not sqlbase.NormalizeName()?
It seems to me that OVER ("Ünicode") FROM t WINDOW "Ünicode" is valid.

---

sql/window.go, line 123 [r1] (raw file):

          // SELECT rank() OVER w FROM t WINDOW w as (...)
          // We use the referenced window specification directly. without modification.
          refName = strings.ToLower(windowDef.Name)

ditto

---

sql/window.go, line 277 [r1] (raw file):

      }
      if !next {
          n.populated = true

Maybe move this assignment to below the checks? That's what you did in group.go already. Unless the two methods use it.

---

sql/window.go, line 355 [r1] (raw file):

// computeWindows populates n.windowValues, adding a column of values to the
// 2D-slice for each window function in n.funcs.
func (n *windowNode) computeWindows() error {

Disclaimer: I do not feel qualified to review the correctness of this function. And probably I'll need a second round reading this, it's really a lot of new SQL stuff for me to page into.

---

sql/parser/aggregate_builtins.go, line 55 [r1] (raw file):

      if t.IsWindowFunction() {
          // A window function is not an aggregate function, but it can
          // contain aggregate functions.

Can you clarify what is meant here. If the node can contain aggregate functions, then I would expect that IsAggregate() should return true if it actually does.

---

sql/parser/aggregate_builtins.go, line 270 [r1] (raw file):

  case *DDecimal:
      count := inf.NewDec(int64(a.count), 0)
      dd := &DDecimal{}

Please move this to a different commit (at the least) or PR (best)

---

sql/parser/aggregate_builtins.go, line 661 [r1] (raw file):

      return NewDFloat(DFloat(math.Sqrt(float64(*t))))
  case *DDecimal:
      dd := &DDecimal{}

ditto

---

sql/parser/select.go, line 503 [r1] (raw file):

  if node.RefName != "" {
      buf.WriteString(node.RefName)
      space = true

What's wrong with doing buf.WriteByte(' ') here and skip this space madness?

---

sql/parser/sql.y, line 3623 [r1] (raw file):

    f := $1.expr().(*FuncExpr)
    f.WindowDef = $4.windowDef()
    $$.val = f

Bleah! Make a helper function at the start of this file and use it. I want to read $$.val = attachWindowDef($1.funcExpr(), $4.windowDef())

---

sql/parser/window.go, line 23 [r1] (raw file):

// ContainsWindowVisitor checks if walked expressions contain window functions.
type ContainsWindowVisitor struct {
  SawWindow bool

I think you can un-export the field since it's only reachable by the methods below.

---

sql/parser/window.go, line 44 [r1] (raw file):

// Reset clear the ContainsWindowVisitor's internal state.
func (v *ContainsWindowVisitor) Reset() {

I don't like this method (see my other review comments) and if it is there to stay the code comment needs to make a better job at convincing the reader why it's needed.

---

sql/parser/window.go, line 51 [r1] (raw file):

func (v *ContainsWindowVisitor) ContainsWindowFunc(expr Expr) bool {
  if expr != nil {
      defer v.Reset()

Wait, no. Allocating a defer here is not warranted. Just do:

WalkExprconst(v, expr)
ret := v.SawWindow
v.SawWindow = false
return ret

Also I do not fully grasp why you need to reset the field to false here; a comment is warranted.

---

sql/parser/window.go, line 67 [r1] (raw file):

// window function, using the Parser's embedded visitor.
func (p *Parser) WindowFuncInExprs(exprs []TypedExpr) bool {
  defer p.containsWindowVisitor.Reset()

Same here. defer is expensive. Simply assign the field before returning. I think this "Reset" method really needs not exist.

---

sql/parser/window.go, line 69 [r1] (raw file):

  defer p.containsWindowVisitor.Reset()
  for _, expr := range exprs {
      WalkExprConst(&p.containsWindowVisitor, expr)

Either reuse WindowFuncInExpr or ContainsWindowFunc or justify in a comment why it's not possible.

---

sql/testdata/window, line 1 [r1] (raw file):

statement ok

Disclaimer: I can't review these tests; I am not (yet) well enough versed with WINDOW functions to check the expected results are all correct. I assume you checked/produced them with sqlite or pg?

---

sql/testdata/window, line 333 [r1] (raw file):

3  3.5355339059327376
8  3.5355339059327376

Please add some tests either here or in testdata/subquery that test subqueries in the various new places. Let's exercise the expand/start logic you have added.

---

Comments from Reviewable

[RaduBerinde]

Wow you have been busy! :) I will need to make multiple passes but the overall structure looks good. As far as memory usage goes, I don't think this is any different than grouping right? (so wrt #8691 we should do whatever we do for grouping)

---

Review status: all files reviewed at latest revision, 25 unresolved discussions, some commit checks failed.

---

sql/window.go, line 30 [r1] (raw file):

Previously, knz (kena) wrote…

Please explain in a comment here your terminology, in particular the difference between:

• window specification
• window definition
• window function
• window function application
• window node
  For each specify where they start to exist in the lifetime of a query and what part of the code is responsible for creating them.

+1 I would also add an example for which the specification, function, definition are described.

---

sql/window.go, line 35 [r1] (raw file):

func (p *planner) window(n *parser.SelectClause, s *selectNode) (*windowNode, error) {
  // Determine if a window-function is being applied. We use the selectNode's
  // renders because window functions will never have be simplified out since

needs rephrasing
Also, can you point to an example where an ORDER BY clause ads a window function? (or point to relevant code which already explains it)

---

sql/window.go, line 72 [r1] (raw file):

          // One or more window functions in render. Create a new render in
          // selectNode for each window function argument.
          window.windowRender[i] = typedExpr

This code had me scratching my head for a while - please explain that the visitor adds the functions it encounters to n.funcs. You could also make this more explicit by:

• doing windowCount := len(n.funcs) before the visitor call (and maybe rename it to oldNumFuncs or something), or
• instead of returning a bool, extract could return the number of functions added; then we could just loop over range n.funcs[len(n.funcs) - numFuncsAdded:], or
• we could put funcs in the visitor and copy them in and out to window.funcs here.

---

sql/window.go, line 328 [r1] (raw file):

  for _, c := range n.ordering {
      var da, db parser.Datum
      if c.Direction == encoding.Ascending {

Can't we compare ra with rb and do return -c if it's not ascending?

---

sql/parser/select.go, line 503 [r1] (raw file):

Previously, knz (kena) wrote…

What's wrong with doing buf.WriteByte(' ') here and skip this space madness?

We don't want the space to show up if it's not necessary e.g. `(PARTITION by X )`.

---

sql/parser/select.go, line 519 [r1] (raw file):

          // We need to remove the initial space produced by OrderBy.Format.
          var tmpBuf bytes.Buffer
          FormatNode(&tmpBuf, f, node.OrderBy)

Not part of this change but maybe we should add a FmtFlag that helps each node know if it needs to prepend a space or not? It would need to be plumbed correctly throughout though (each function needs to pass a potentially different value to any format calls it makes)
@knz

---

Comments from Reviewable

[knz]

@RaduBerinde it's different from grouping because with grouping we're not storing all the rows in each group (the grouped rows are streamed through each group's aggregator and then discarded; the only memory budget is for the small aggregator struct itself, one per group). With WINDOW we keep all the rows in memory for each partition. That can grow huge.

---

Review status: all files reviewed at latest revision, 25 unresolved discussions, some commit checks failed.

---

sql/window.go, line 72 [r1] (raw file):

Previously, RaduBerinde wrote…

This code had me scratching my head for a while - please explain that the visitor adds the functions it encounters to n.funcs. You could also make this more explicit by:

• doing windowCount := len(n.funcs) before the visitor call (and maybe rename it to oldNumFuncs or something), or
• instead of returning a bool, extract could return the number of functions added; then we could just loop over range n.funcs[len(n.funcs) - numFuncsAdded:], or
• we could put funcs in the visitor and copy them in and out to window.funcs here.

👍

---

sql/parser/select.go, line 503 [r1] (raw file):

Previously, RaduBerinde wrote…

We don't want the space to show up if it's not necessary e.g. (PARTITION by X ).

Aha, then I'd suggest the name to be `needSpaceSeparator` for clarity.

---

sql/parser/select.go, line 519 [r1] (raw file):

Previously, RaduBerinde wrote…

Not part of this change but maybe we should add a FmtFlag that helps each node know if it needs to prepend a space or not? It would need to be plumbed correctly throughout though (each function needs to pass a potentially different value to any format calls it makes)
@knz

Wow that would be .... challenging. I'd rather change the interface to use io.Writer then use a custom writer that tracks spaces and provide a method to "append a space before the following text if and only if there is no space already there". But that's definitely for another PR.

---

Comments from Reviewable

[RaduBerinde]

Ah, I see. Then s/grouping/sorting :)

---

Review status: all files reviewed at latest revision, 25 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[nvanbenschoten]

TFTRs! I'll wait for #8691 to land and rebase ontop of it so as not to step on @knz's toes.

With regard to the the the memory usage, I also dont see why this would be a big issue. Like @RaduBerinde said, this should be comparable to sorting with a linear memory usage with respect to row count, unlike the quadratic memory usage blowup of joins.

---

Review status: all files reviewed at latest revision, 25 unresolved discussions, some commit checks failed.

---

sql/window.go, line 30 [r1] (raw file):

Previously, RaduBerinde wrote…

+1 I would also add an example for which the specification, function, definition are described.

Good point. Is that description sufficient?

---

sql/window.go, line 35 [r1] (raw file):

Previously, RaduBerinde wrote…

needs rephrasing
Also, can you point to an example where an ORDER BY clause ads a window function? (or point to relevant code which already explains it)

Done.

---

sql/window.go, line 72 [r1] (raw file):

Previously, knz (kena) wrote…

👍

Done.

---

sql/window.go, line 111 [r1] (raw file):

Previously, knz (kena) wrote…

Explain why / when overriding is necessary.

Done.

---

sql/window.go, line 114 [r1] (raw file):

Previously, knz (kena) wrote…

I think for readability this switch and the test on refName below could be moved to their own function.

Done.

---

sql/window.go, line 118 [r1] (raw file):

Previously, knz (kena) wrote…

Why not sqlbase.NormalizeName()?
It seems to me that OVER ("Ünicode") FROM t WINDOW "Ünicode" is valid.

Done. Using `parser.Name` throughout for these names now too.

---

sql/window.go, line 123 [r1] (raw file):

Previously, knz (kena) wrote…

ditto

Done.

---

sql/window.go, line 277 [r1] (raw file):

Previously, knz (kena) wrote…

Maybe move this assignment to below the checks? That's what you did in group.go already. Unless the two methods use it.

I actually just moved it above in `group.go`. It was being set twice there for some reason, and if its only set below, we miss it on error.

---

sql/window.go, line 328 [r1] (raw file):

Previously, RaduBerinde wrote…

Can't we compare ra with rb and do return -c if it's not ascending?

We most certainly can. Done.

---

sql/window.go, line 355 [r1] (raw file):

Previously, knz (kena) wrote…

Disclaimer: I do not feel qualified to review the correctness of this function. And probably I'll need a second round reading this, it's really a lot of new SQL stuff for me to page into.

Yeah I understand. The PG docs do a pretty good job explaining window functions, although they're scattered over a few pages: - https://www.postgresql.org/docs/9.5/static/tutorial-window.html - https://www.postgresql.org/docs/9.5/static/functions-window.html - https://www.postgresql.org/docs/9.5/static/sql-expressions.html#SYNTAX-WINDOW-FUNCTIONS - https://www.postgresql.org/docs/9.5/static/queries-table-expressions.html#QUERIES-WINDOW

I also found this yesterday, which is pretty succinct:

• https://cloud.google.com/bigquery/query-reference#windowfunctions

---

sql/window.go, line 465 [r1] (raw file):

Previously, mjibson (Matt Jibson) wrote…

populated -> populates

Done.

---

sql/parser/aggregate_builtins.go, line 55 [r1] (raw file):

Previously, knz (kena) wrote…

Can you clarify what is meant here. If the node can contain aggregate functions, then I would expect that IsAggregate() should return true if it actually does.

This is in the `IsAggregateVisitor`'s `VisitPre` method, so `return true, expr` just means to recurse into the expression, but avoid the rest of this function for the current node.

---

sql/parser/aggregate_builtins.go, line 270 [r1] (raw file):

Previously, knz (kena) wrote…

Please move this to a different commit (at the least) or PR (best)

Yeah good point. See #9089.

---

sql/parser/select.go, line 503 [r1] (raw file):

Previously, knz (kena) wrote…

Aha, then I'd suggest the name to be needSpaceSeparator for clarity.

Done.

---

sql/parser/sql.y, line 3623 [r1] (raw file):

Previously, knz (kena) wrote…

Bleah! Make a helper function at the start of this file and use it. I want to read $$.val = attachWindowDef($1.funcExpr(), $4.windowDef())

For the one instance of this? Seems like more work than it's worth, especially because we don't have helpers like this anywhere else.

---

sql/parser/sql.y, line 3740 [r1] (raw file):

Previously, mjibson (Matt Jibson) wrote…

Unnecessary semicolon

Done.

---

sql/parser/window.go, line 23 [r1] (raw file):

Previously, knz (kena) wrote…

I think you can un-export the field since it's only reachable by the methods below.

Done.

---

sql/parser/window.go, line 44 [r1] (raw file):

Previously, knz (kena) wrote…

I don't like this method (see my other review comments) and if it is there to stay the code comment needs to make a better job at convincing the reader why it's needed.

Done.

---

sql/parser/window.go, line 51 [r1] (raw file):

Previously, knz (kena) wrote…

Wait, no. Allocating a defer here is not warranted. Just do:

WalkExprconst(v, expr)
ret := v.SawWindow
v.SawWindow = false
return ret

Also I do not fully grasp why you need to reset the field to false here; a comment is warranted.

Done. The reason we should reset it is so that the visitor can be resused.

Also, a lot of this was structured to mirror IsAggregateVisitor. I'll follow up this PR with a change to make IsAggregateVisitor reflect these preferences as well.

---

sql/parser/window.go, line 67 [r1] (raw file):

Previously, knz (kena) wrote…

Same here. defer is expensive. Simply assign the field before returning. I think this "Reset" method really needs not exist.

Done.

---

sql/parser/window.go, line 69 [r1] (raw file):

Previously, knz (kena) wrote…

Either reuse WindowFuncInExpr or ContainsWindowFunc or justify in a comment why it's not possible.

The reason was to have a single `Defer` (again, see `IsAggregateVisitor`). Changed.

---

sql/testdata/window, line 1 [r1] (raw file):

Previously, knz (kena) wrote…

Disclaimer: I can't review these tests; I am not (yet) well enough versed with WINDOW functions to check the expected results are all correct. I assume you checked/produced them with sqlite or pg?

All of this was verified with PG.

---

sql/testdata/window, line 333 [r1] (raw file):

Previously, knz (kena) wrote…

Please add some tests either here or in testdata/subquery that test subqueries in the various new places. Let's exercise the expand/start logic you have added.

Done. Added a few subquery tests, again verified with PG.

---

Comments from Reviewable

[knz]

This looks good. I'll have a last review round after I finish reading the PG docs. Hopefully before Monday :)

---

Reviewed 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

sql/window.go, line 35 [r2] (raw file):

// This code uses the following terminology throughout:
// - built-in window functions:
//     a set of built-in functions that can only be used in the context of a window

Either define "window" here or give a link to some external documentation that explains what windows are.

---

sql/parser/aggregate_builtins.go, line 55 [r1] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This is in the IsAggregateVisitor's VisitPre method, so return true, expr just means to recurse into the expression, but avoid the rest of this function for the current node.

Thanks

---

Comments from Reviewable

[knz]

Ok now I understand what windows do and what the code does.
Perf wise and memory wise it's not as terrible as join indeed, but it's much worse than group/sort, because you potentially create a copy of all the rows for each window definition in the query, so the space complexity is O(N_W) (W = number of windows) and the time complexity is N_W (no ordering) and W*NlogN (with ordering)
This needs to be properly documented, and perhaps a new blog post could explain this like the JOIN post did.

Please file an issue to optimize the case where the data source is already ordered by the partition expression, in which case we only need to hold the rows for 1 partition at a time in memory. Also if the data source is already ordered by both the partition expression and the partition's order by clause we only need to hold 1 row at a time.

otherwise.

---

Review status: all files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

---

sql/window.go, line 355 [r1] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Yeah I understand. The PG docs do a pretty good job explaining window functions, although they're scattered over a few pages:

• https://www.postgresql.org/docs/9.5/static/tutorial-window.html
• https://www.postgresql.org/docs/9.5/static/functions-window.html
• https://www.postgresql.org/docs/9.5/static/sql-expressions.html#SYNTAX-WINDOW-FUNCTIONS
• https://www.postgresql.org/docs/9.5/static/queries-table-expressions.html#QUERIES-WINDOW

I also found this yesterday, which is pretty succinct:

• https://cloud.google.com/bigquery/query-reference#windowfunctions

Thanks, that helped.

---

sql/window.go, line 142 [r2] (raw file):

      // TODO(nvanbenschoten) below we add renders to the selectNode for each
      // partition and order expression. We should handle cases where the expression

Can you file an issue to follow up on this.

---

sql/window.go, line 245 [r2] (raw file):

  // an entire column in windowValues for each windowFuncHolder, in order.
  funcs        []*windowFuncHolder
  windowValues [][]parser.Datum

windowValue's outer array is indexed by the position of the window function in funcs, right?
(So this will become []RowContainer after #8691?)

---

sql/window.go, line 410 [r2] (raw file):

      //
      // TODO(nvanbenschoten) Window functions with the same window definition
      // can share partition and sorting work.

Well yes and it's worse than this, here you duplicate the work even if the original query made it clear it was the same window (a over w), (b over w) window w ....

Why not having one partition work per window definition, and then as 2nd step let the window functions reuse the partitions based on the name?

For windows without an explicit name you could use the syntax in the query as name, that would buy you automatic reuse of the window if you have 2 or more times the same def.

---

sql/window.go, line 468 [r2] (raw file):

              // sorter as our peerGroupChecker.
              sorter := &partitionSorter{rows: partition, ordering: windowFn.columnOrdering}
              // The sort needs to be stable because multiple window functions with

I don't understand why it needs to be stable. To my eyes it merely needs to be deterministic, so you get the same ordering in every window definition.

---

sql/window.go, line 516 [r2] (raw file):

  for i := range n.values.rows {
      curRow := make(parser.DTuple, len(n.windowRender))

I'd suggest optimizing this by allocating a single array of dtuples with size len(windowRender)*len(wrappedValues)

---

sql/parser/sql.y, line 3623 [r1] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

For the one instance of this? Seems like more work than it's worth, especially because we don't have helpers like this anywhere else.

Your call.

---

Comments from Reviewable

[nvanbenschoten]

Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

---

sql/window.go, line 35 [r2] (raw file):

Previously, knz (kena) wrote…

Either define "window" here or give a link to some external documentation that explains what windows are.

Done.

---

sql/window.go, line 142 [r2] (raw file):

Previously, knz (kena) wrote…

Can you file an issue to follow up on this.

Done. #9173.

---

sql/window.go, line 245 [r2] (raw file):

Previously, knz (kena) wrote…

windowValue's outer array is indexed by the position of the window function in funcs, right?
(So this will become []RowContainer after #8691?)

I believe it will become `RowContainer` (not a slice of them), right?

---

sql/window.go, line 410 [r2] (raw file):

Previously, knz (kena) wrote…

Well yes and it's worse than this, here you duplicate the work even if the original query made it clear it was the same window (a over w), (b over w) window w ....

Why not having one partition work per window definition, and then as 2nd step let the window functions reuse the partitions based on the name?

For windows without an explicit name you could use the syntax in the query as name, that would buy you automatic reuse of the window if you have 2 or more times the same def.

The referenced paper goes into this in detail, so I'd rather focus an entire PR on improving this efficiency as much as possible. For now I'm not going to worry about optimizations like this, especially if it means adding complicated logic that isnt complete.

---

sql/window.go, line 468 [r2] (raw file):

Previously, knz (kena) wrote…

I don't understand why it needs to be stable. To my eyes it merely needs to be deterministic, so you get the same ordering in every window definition.

Yeah you're right. I hadnt checked if `sort.Sort` contained some sort of randomized quicksort which would make the ordering of equal elements nondeterministic, but it doesnt. Switched back.

---

sql/window.go, line 516 [r2] (raw file):

Previously, knz (kena) wrote…

I'd suggest optimizing this by allocating a single array of dtuples with size len(windowRender)*len(wrappedValues)

Done.

---

Comments from Reviewable

[knz]

Reviewed 9 of 9 files at r3.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks pending.

---

sql/window.go, line 245 [r2] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I believe it will become RowContainer (not a slice of them), right?

Correct.

---

sql/window.go, line 410 [r2] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

The referenced paper goes into this in detail, so I'd rather focus an entire PR on improving this efficiency as much as possible. For now I'm not going to worry about optimizations like this, especially if it means adding complicated logic that isnt complete.

Ok, works for me.

---

Comments from Reviewable