sql: Add support for specifying window frames for window functions

[yuzefovich]

WIP on #26464:
ROWS mode is fully supported whereas RANGE works only with
UNBOUNDED PRECEDING/CURRENT ROW/UNBOUNDED FOLLOWING boundaries
(same as in PostgreSQL). Current implementation of aggregate functions
is naive (it simply computes the value of aggregate directly and
discards all the previous computations which results in quadratic time).

Release note: None

[cockroach-teamcity]

This change is 

[knz]

(nit: As this is a user-visible change you'll need to populate a release note in the commit message(s) before this merges.)

[knz]

Review status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/sem/tree/walk.go, line 217 at r1 (raw file):

			windowFrameCopy := *windowDef.Frame
			windowDef.Frame = &windowFrameCopy
			if windowFrameBoundsCopy := *windowDef.Frame.Bounds; &windowFrameBoundsCopy != nil {

this if is problematic. I think you mean this:

if windowDef.Frame.Bounds != nil {
    windowFrameBoundsCopy := *windowDef.Frame.Bounds
    windowDef.Frame.Bounds = &windowFrameBoundsCopy
}

---

Comments from Reviewable

[knz]

Reviewed 9 of 10 files at r1.
Review status: complete! 0 of 0 LGTMs obtained

---

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

		bounds := frame.Bounds
		if bounds.StartBound.BoundType == tree.ValuePreceding || bounds.StartBound.BoundType == tree.ValueFollowing {
			typedStartOffset := bounds.StartBound.TypedOffsetExpr

Do this instead:
typedStarOffset := bounds.StartBound.OffsetExpr.(tree.TypedExpr)

no need for a separate field. Ditto below.

---

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

				funcHolder.funcIdx = prevWindowCount + i
				funcHolder.argIdxStart = len(newRenders)
				funcHolder.windowDef = *funcHolder.expr.WindowDef

You're making a copy of WindowDef here. If you are making this copy before the calls to analyzeExpr() in constructWindowDefinitions, then later funcHolder.windowDef will not contain the proper expressions. This is the main reason why you encounter the subquery error.

Remember that

1. expression trees, once constructed, must be immutable. We reuse trees across multiple executions of a statement so the parsed tree must never be mutated in-place.

2. analyzeExpr rewrites the expression tree to produce the typed expression. (You can verify this by checking that the address of the root expression node has changed.)

I believe you need to analyze the WindowDef just once (and all its sub-expressions) and then memoize the result somehow.

---

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

		}

		n.run.windowFrame = windowDef.Frame

Make a copy of both the frame and bounds object because you are mutating them below. As per my comment above the original syntax tree must remain immutable.

---

pkg/sql/parser/sql.y, line 7177 at r1 (raw file):

    switch {
    case startBound.BoundType == tree.UnboundedFollowing:
      sqllex.Error("frame start cannot be UNBOUNDED FOLLOWING")

This needs both positive and negative tests in parse_test.go.

---

pkg/sql/sem/tree/window_funcs.go, line 54 at r1 (raw file):

	Offset          int
	OffsetExpr      Expr
	TypedOffsetExpr TypedExpr

Don't make a separate field for the typed expression. Reuse OffsetExpr directly.

---

Comments from Reviewable

[yuzefovich]

Review status: complete! 0 of 0 LGTMs obtained

---

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

Previously, knz (kena) wrote…

Do this instead:
typedStarOffset := bounds.StartBound.OffsetExpr.(tree.TypedExpr)

no need for a separate field. Ditto below.

Done.

---

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

Previously, knz (kena) wrote…

You're making a copy of WindowDef here. If you are making this copy before the calls to analyzeExpr() in constructWindowDefinitions, then later funcHolder.windowDef will not contain the proper expressions. This is the main reason why you encounter the subquery error.

Remember that

1. expression trees, once constructed, must be immutable. We reuse trees across multiple executions of a statement so the parsed tree must never be mutated in-place.

2. analyzeExpr rewrites the expression tree to produce the typed expression. (You can verify this by checking that the address of the root expression node has changed.)

I believe you need to analyze the WindowDef just once (and all its sub-expressions) and then memoize the result somehow.

Thanks for the detailed explanation - it helped me find a bug (I called analyzeExpr on a copy of parsed tree but later was using the original parsed tree).

---

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

Previously, knz (kena) wrote…

Make a copy of both the frame and bounds object because you are mutating them below. As per my comment above the original syntax tree must remain immutable.

Done.

---

pkg/sql/parser/sql.y, line 7177 at r1 (raw file):

Previously, knz (kena) wrote…

This needs both positive and negative tests in parse_test.go.

Done.

---

pkg/sql/sem/tree/walk.go, line 217 at r1 (raw file):

Previously, knz (kena) wrote…

this if is problematic. I think you mean this:

if windowDef.Frame.Bounds != nil {
    windowFrameBoundsCopy := *windowDef.Frame.Bounds
    windowDef.Frame.Bounds = &windowFrameBoundsCopy
}

Done.

---

pkg/sql/sem/tree/window_funcs.go, line 54 at r1 (raw file):

Previously, knz (kena) wrote…

Don't make a separate field for the typed expression. Reuse OffsetExpr directly.

Done.

---

Comments from Reviewable

[nvanbenschoten]

Reviewed 6 of 10 files at r1, 1 of 6 files at r2.
Review status: complete! 0 of 0 LGTMs obtained

---

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

	// OffsetExpr's must be integer expressions not containing any variables, aggregate functions, or window functions,
	// so we need to make sure these expressions are evaluated before using offsets.

Where are you enforcing this? MustBeDInt will panic if the resulting datum is not a DInt, but I don't see any other type checking that forces the type (just one that desired it).

---

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

	if frame != nil && frame.Bounds != nil {
		bounds := frame.Bounds
		if bounds.StartBound.BoundType == tree.ValuePreceding || bounds.StartBound.BoundType == tree.ValueFollowing {

This is leaking an abstraction. Either make HasExpr a method on WindowFrameBoundType or just check if the expression is nil or not.

---

pkg/sql/logictest/testdata/logic_test/window, line 1518 at r1 (raw file):

SELECT final_variance(1.2, 1.2, 123) OVER (PARTITION BY k) FROM kv

I'm assuming you tested all of this against PG as well?

---

pkg/sql/logictest/testdata/logic_test/window, line 1543 at r1 (raw file):

('Tablet', 'Samsung', 200, 200, 200)

statement error cannot copy window "w" because it has a frame clause

Add tests where you're given a non-int start and end bounds.

---

pkg/sql/parser/sql.y, line 7183 at r1 (raw file):

      return 1
    case startBound.BoundType == tree.CurrentRow && endBound.BoundType == tree.ValuePreceding:
      sqllex.Error("frame starting from current row cannot have preceding rows")

Are these errors straight from PG?

---

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

}

// framableAggregateWindowFunc is a wrapper around aggregateWindowFunc that allows

Will this abstraction be useful when we look to break the current quadratic behavior by performing some memoization using a segment tree?

---

pkg/sql/sem/tree/window_funcs.go, line 1 at r1 (raw file):

// Copyright 2017 The Cockroach Authors.

For the sake of reviewability, I'd strongly suggest you go back and make the filename fix in a commit where you touch nothing else.

---

pkg/sql/sem/tree/window_funcs.go, line 52 at r1 (raw file):

type WindowFrameBound struct {
	BoundType       WindowFrameBoundType
	Offset          int

I'm also not sure we need this. OffsetExpr should be all we need.

---

pkg/sql/sem/tree/window_funcs.go, line 60 at r1 (raw file):

// The row at StartBound is included whereas the row at EndBound is not.
type WindowFrameBounds struct {
	StartBound *WindowFrameBound

Does this need to be nullable? Looks like it's always non-nil if WindowFrameBounds is non-nil.

---

pkg/sql/sem/tree/window_funcs.go, line 65 at r1 (raw file):

// WindowFrame is a view into a subset of data over which calculations are made.
type WindowFrame struct {

I think we're misconstruing two concepts here. WindowFrame is and was used as an object to hold state between calls to a WindowFunc interface during evaluation. It's not an AST node, which for all intents and purposes should be immutable and contain no dynamic state. We're going to want to lift a lot of the new code here up to live next to WindowDef.

---

pkg/sql/sem/tree/window_funcs.go, line 86 at r1 (raw file):

		ctx.WriteString("UNBOUNDED PRECEDING")
	case ValuePreceding:
		ctx.WriteString(fmt.Sprintf("%v PRECEDING", boundary.Offset))

Replace here and below with

ctx.FormatNode(boundary.OffsetExpr)
ctx.WriteString(" PRECEDING")

---

pkg/sql/sem/tree/window_funcs.go, line 93 at r1 (raw file):

	case UnboundedFollowing:
		ctx.WriteString("UNBOUNDED FOLLOWING")
	}

Go doesn't statically enforce that switch statements are exhaustive. It's generally good practice to do so with a default case like:

default:
    panic("unexpected")

I'd add those throughout.

---

Comments from Reviewable

[yuzefovich]

Review status: complete! 0 of 0 LGTMs obtained

---

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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Where are you enforcing this? MustBeDInt will panic if the resulting datum is not a DInt, but I don't see any other type checking that forces the type (just one that desired it).

TypeCheckAndRequire was called as a part of analyzeExpr call within constructWindowDefinitions. However, with Nathan's help I realized that analyzeExpr is not necessary and was there because I had a bug earlier. Hopefully, new version is the way to go.

---

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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This is leaking an abstraction. Either make HasExpr a method on WindowFrameBoundType or just check if the expression is nil or not.

Done.

---

pkg/sql/logictest/testdata/logic_test/window, line 1518 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I'm assuming you tested all of this against PG as well?

Yes, the only differences I found in the output are the number of zeros that PG and CRDB return in number like 200.00 and printing out NULL values (PG leaves them empty), but I think that these are expected behavior, right?

---

pkg/sql/logictest/testdata/logic_test/window, line 1543 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Add tests where you're given a non-int start and end bounds.

Done.

---

pkg/sql/parser/sql.y, line 7183 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Are these errors straight from PG?

Yes, I manually tried all these combination to get their error messages so that ours would be consistent with them.

---

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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Will this abstraction be useful when we look to break the current quadratic behavior by performing some memoization using a segment tree?

I'm not sure, to be honest. I plan on adding a method that would replace current aggregator with faster implementation (via switch on type of the current aggregator like avg). In that case, aggConstructor would be set to nil to use advantage of memoization. However, this abstraction allows us to add fast implementations of some (say, avg, min, max) aggregate functions and to go back to slow quadratic behavior if faster is not present.

---

pkg/sql/sem/tree/window_funcs.go, line 1 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

For the sake of reviewability, I'd strongly suggest you go back and make the filename fix in a commit where you touch nothing else.

Good idea.

---

pkg/sql/sem/tree/window_funcs.go, line 52 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I'm also not sure we need this. OffsetExpr should be all we need.

As we discussed, OffsetExpr is evaluated only once and the result is cached in Offset.

---

pkg/sql/sem/tree/window_funcs.go, line 60 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Does this need to be nullable? Looks like it's always non-nil if WindowFrameBounds is non-nil.

Correct, StartBound is always non-nil, but making it an instance instead of a pointer would require changes in the parser (I believe we would need to handle startBound and endBound differently since they would be returning different types which would require some copy/pasting), so I think it's better to keep it as a pointer. (Possibly I'm not seeing a better way to go about changes in the parser.)

---

pkg/sql/sem/tree/window_funcs.go, line 65 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I think we're misconstruing two concepts here. WindowFrame is and was used as an object to hold state between calls to a WindowFunc interface during evaluation. It's not an AST node, which for all intents and purposes should be immutable and contain no dynamic state. We're going to want to lift a lot of the new code here up to live next to WindowDef.

Ok, I divided it into two structs. Please let me know if further refinements are desirable (and give suggestions if so).

---

pkg/sql/sem/tree/window_funcs.go, line 86 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Replace here and below with

ctx.FormatNode(boundary.OffsetExpr)
ctx.WriteString(" PRECEDING")

Done.

---

pkg/sql/sem/tree/window_funcs.go, line 93 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Go doesn't statically enforce that switch statements are exhaustive. It's generally good practice to do so with a default case like:

default:
    panic("unexpected")

I'd add those throughout.

Done.

---

Comments from Reviewable

[jordanlewis]

Generally looks good, and nice tests. once comments get resolved. Looks like you'll need to rebase this against master as there's some merge conflicts.

---

Reviewed 2 of 10 files at r1, 1 of 6 files at r2, 8 of 9 files at r3.
Review status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/sql/window.go, line 428 at r3 (raw file):

	if referencedSpec.Frame != nil {
		return def, errors.Errorf("cannot copy window %q because it has a frame clause", refName)

you should use a pgerror here, to match Postgres. In psql, if you enter \set VERBOSITY verbose before running commands, you will see specific error codes depending on the error type. This one happens to return 42P20, windowing error, which can be retrieved in Cockroach by saying

pgerror.NewErrorf(pgerror.CodeWindowingError, "cannot copy window %q because it has a frame clause", refName)

---

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

Previously, yuzefovich wrote…

I'm not sure, to be honest. I plan on adding a method that would replace current aggregator with faster implementation (via switch on type of the current aggregator like avg). In that case, aggConstructor would be set to nil to use advantage of memoization. However, this abstraction allows us to add fast implementations of some (say, avg, min, max) aggregate functions and to go back to slow quadratic behavior if faster is not present.

I think it'll be useful to keep around as well for testing purposes. THis is a straightforward implementation that will be useful as a testing baseline, in case it's difficult to implement the more efficient algorithms.

---

pkg/sql/sem/tree/window_funs.go, line 113 at r3 (raw file):

// Copy returns a deep copy of wf.
func (wf WindowFrame) Copy() *WindowFrame {

minor: It seems odd that the receiver is not a pointer but it is for Format. I don't know if it matters much, but for consistency I'd make the receiver a pointer as well.

---

pkg/sql/sem/tree/window_funs.go, line 132 at r3 (raw file):

	ArgIdxStart int // the index which arguments to the window function begin
	ArgCount    int // the number of window function arguments
	Frame       *WindowFrame

Add a comment above this new field explaining what this is. If it can be nil, say so - something like If non-nil, Frame represents the frame specification for this window.

---

pkg/sql/sem/tree/window_funs.go, line 153 at r3 (raw file):

			return 0
		case ValuePreceding:
			// TODO(yuzefovich): Currently, it is not supported, and this case should not be reached.

minor: I would just panic in these situations instead of returning 0, to make it blindingly clear that these won't be reached, but it's not a big deal either way.

---

Comments from Reviewable

[yuzefovich]

Review status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/sql/window.go, line 428 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

you should use a pgerror here, to match Postgres. In psql, if you enter \set VERBOSITY verbose before running commands, you will see specific error codes depending on the error type. This one happens to return 42P20, windowing error, which can be retrieved in Cockroach by saying

pgerror.NewErrorf(pgerror.CodeWindowingError, "cannot copy window %q because it has a frame clause", refName)

Done.

---

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

Previously, jordanlewis (Jordan Lewis) wrote…

I think it'll be useful to keep around as well for testing purposes. THis is a straightforward implementation that will be useful as a testing baseline, in case it's difficult to implement the more efficient algorithms.

I agree.

---

pkg/sql/sem/tree/window_funs.go, line 113 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

minor: It seems odd that the receiver is not a pointer but it is for Format. I don't know if it matters much, but for consistency I'd make the receiver a pointer as well.

Done.

---

pkg/sql/sem/tree/window_funs.go, line 132 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Add a comment above this new field explaining what this is. If it can be nil, say so - something like If non-nil, Frame represents the frame specification for this window.

Done.

---

Comments from Reviewable

[nvanbenschoten]

This is looking better and better. Nice job so far.

---

Reviewed 10 of 11 files at r4.
Review status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/sql/window.go, line 357 at r4 (raw file):

		n.run.windowFrame = windowDef.Frame
		// Validate window frame bounds if present
		frame := n.run.windowFrame

Why are we type checking again? We shouldn't need to perform any type checking in this file.

---

pkg/sql/logictest/testdata/logic_test/window, line 1518 at r1 (raw file):

Previously, yuzefovich wrote…

Yes, the only differences I found in the output are the number of zeros that PG and CRDB return in number like 200.00 and printing out NULL values (PG leaves them empty), but I think that these are expected behavior, right?

Yep, both of those are expected divergences.

---

pkg/sql/sem/builtins/window_builtins.go, line 206 at r4 (raw file):

	// and construct a new one for the computation.
	w.agg.Close(ctx, evalCtx)
	w.agg = &aggregateWindowFunc{agg: w.aggConstructor(evalCtx)}

Don't reallocate this, just reassign the value: *w.agg = aggregateWindowFunc{...}

---

pkg/sql/sem/builtins/window_builtins.go, line 234 at r4 (raw file):

}

// AddAggregateConstructorToFramableAggregate add provided constructor to framableAggregateWindowFunc

Is this modifying the builtin itself? Is that safe? Aren't these static objects?

---

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

	windowFunc tree.WindowFunc, aggConstructor func(*tree.EvalContext) tree.AggregateFunc,
) {
	if framableAgg, ok := windowFunc.(*framableAggregateWindowFunc); ok {

It's ok if windowFunc is not a *framableAggregateWindowFunc?

---

pkg/sql/sem/tree/type_check.go, line 765 at r4 (raw file):

			startBound, endBound := bounds.StartBound, bounds.EndBound
			if startBound.OffsetExpr != nil {
				typedStartOffsetExpr, err := startBound.OffsetExpr.TypeCheck(ctx, types.Int)

I'm still not sure how we're getting away with not replacing this line with

typedStartOffsetExpr, err := typeCheckAndRequire(ctx, startBound.OffsetExpr, types.Int, "window frame offset")

---

pkg/sql/sem/tree/window_funs.go, line 268 at r4 (raw file):

}

func (wfr WindowFrameRun) unboundedFollowing() int {

Add a comment to this method to explain it's meaning.

---

pkg/sql/sem/tree/window_funcs.go, line 1 at r1 (raw file):

Previously, yuzefovich wrote…

Good idea.

Thanks, that helps a lot.

---

pkg/sql/sem/tree/window_funcs.go, line 52 at r1 (raw file):

Previously, yuzefovich wrote…

As we discussed, OffsetExpr is evaluated only once and the result is cached in Offset.

But then Offset should be part WindowFrameRun.

---

pkg/sql/sem/tree/window_funcs.go, line 60 at r1 (raw file):

Previously, yuzefovich wrote…

Correct, StartBound is always non-nil, but making it an instance instead of a pointer would require changes in the parser (I believe we would need to handle startBound and endBound differently since they would be returning different types which would require some copy/pasting), so I think it's better to keep it as a pointer. (Possibly I'm not seeing a better way to go about changes in the parser.)

Ok, that makes sense.

---

pkg/sql/sem/tree/window_funcs.go, line 65 at r1 (raw file):

Previously, yuzefovich wrote…

Ok, I divided it into two structs. Please let me know if further refinements are desirable (and give suggestions if so).

Thanks! I would suggest that you collocate WindowFrame and all of its related AST types with Window and WindowDef. I don't have a strong opinion about which file they're in, only that they're in the same file.

---

Comments from Reviewable

[yuzefovich]

Review status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/sql/window.go, line 357 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Why are we type checking again? We shouldn't need to perform any type checking in this file.

You're right - calling TypeCheckAndRequire within type_check.go should be sufficient.

---

pkg/sql/sem/builtins/window_builtins.go, line 206 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Don't reallocate this, just reassign the value: *w.agg = aggregateWindowFunc{...}

Done.

---

pkg/sql/sem/builtins/window_builtins.go, line 234 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is this modifying the builtin itself? Is that safe? Aren't these static objects?

The builtins are not modified, and I believe this to be safe - we simply create a new instance of a builtin (if necessary) to achieve "resetting" behavior. As far as I understand, these objects are not static, i.e. we can create as many as we want without new ones influencing the old ones.

---

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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

It's ok if windowFunc is not a *framableAggregateWindowFunc?

Yes, it's ok since I've amended all builtins specific to window functions (i.e. non-aggregates, but functions like rank) to support window frames "natively". In a sense, those functions are not framable aggregates, so they should not be of that type. I added this comment to the code.

---

pkg/sql/sem/tree/type_check.go, line 765 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I'm still not sure how we're getting away with not replacing this line with

typedStartOffsetExpr, err := typeCheckAndRequire(ctx, startBound.OffsetExpr, types.Int, "window frame offset")

Thanks for pointing it out. Indeed, type checking and requiring Int in here is sufficient.

---

pkg/sql/sem/tree/window_funs.go, line 268 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Add a comment to this method to explain it's meaning.

Done.

---

pkg/sql/sem/tree/window_funcs.go, line 1 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Thanks, that helps a lot.

Done.

---

pkg/sql/sem/tree/window_funcs.go, line 52 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

But then Offset should be part WindowFrameRun.

Done.

---

pkg/sql/sem/tree/window_funcs.go, line 65 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Thanks! I would suggest that you collocate WindowFrame and all of its related AST types with Window and WindowDef. I don't have a strong opinion about which file they're in, only that they're in the same file.

I moved everything into select.go since window functions can only be used with SELECT.

---

Comments from Reviewable

[nvanbenschoten]

---

Reviewed 4 of 7 files at r5.
Review status: complete! 0 of 0 LGTMs obtained (and 2 stale)

---

pkg/sql/sem/tree/type_check.go, line 765 at r5 (raw file):

			startBound, endBound := bounds.StartBound, bounds.EndBound
			if startBound.OffsetExpr != nil {
				typedStartOffsetExpr, err := TypeCheckAndRequire(startBound.OffsetExpr, ctx, types.Int, "window frame start")

I'm not sure when TypeCheckAndRequire (exported) was introduced, but this should use typeCheckAndRequire (not exported).

---

Comments from Reviewable

[yuzefovich]

TFTRs.

---

Review status: complete! 0 of 0 LGTMs obtained (and 2 stale)

---

pkg/sql/sem/tree/type_check.go, line 765 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I'm not sure when TypeCheckAndRequire (exported) was introduced, but this should use typeCheckAndRequire (not exported).

Done.

---

Comments from Reviewable

[yuzefovich]

bors r+

[craig]

Merge conflict (retrying...)

[yuzefovich]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)