sqlsmith: add window frames #36896
sqlsmith: add window frames #36896
Conversation
|
This change is |
|
I wasn't sure whether we want to generate frames that would result in an error (for example, |
pkg/internal/sqlsmith/scalar.go
Outdated
| func makeWindowFrame() *tree.WindowFrame { | ||
| // Window frame mode and start bound must always be present whereas end | ||
| // bound can be omitted. | ||
| frameMode := tree.WindowFrameMode(rand.Intn(3)) |
There was a problem hiding this comment.
Use the pattern similar to randDirection for this. You'll have to pass this func a s *scope argument. I guess BoundType is fine because of how you need to do the slicing for endBounds.
pkg/internal/sqlsmith/scalar.go
Outdated
| @@ -373,6 +379,76 @@ func makeFunc(s *scope, ctx Context, typ types.T, refs colRefs) (tree.TypedExpr, | |||
| ), typ), true | |||
| } | |||
|
|
|||
| const maxWindowFrameOffset = 10 | |||
|
|
|||
| func makeWindowFrame() *tree.WindowFrame { | |||
There was a problem hiding this comment.
pass a s *scope here and use s.schema.rnd.Intn instead of the rand.Intn uses.
pkg/internal/sqlsmith/scalar.go
Outdated
| // We will set offsets regardless of the bound type, but they will only be | ||
| // used when a bound is either OffsetPreceding or OffsetFollowing. Both | ||
| // ROWS and GROUPS mode need non-negative integers as bounds. | ||
| startBound.OffsetExpr = tree.NewDInt(tree.DInt(rand.Intn(maxWindowFrameOffset))) |
There was a problem hiding this comment.
So this is where sqlsmith gets cool. Instead of hard coding an integer, use makeScalar(s, types.Int, refs), and take a refs colRefs argument to this func. It'll produce some funky thing that spits out on int based on the currently available column names, or make some constants.
|
And to answer your question: if it's not too much work I try to make sqlsmith smart so that it doesn't get caught at errors like "you can't do blergh because you are using feezbop", because we want it to get beyond that step and actually execute. However that's hard to do in all cases, so it's fine to leave frames that generate known errors for now. We can clean them up later. I go through from time to time and address those. |
Adds window frames to window functions. RANGE mode of framing is not supported with offset-type bounds. Release note: None
There was a problem hiding this comment.
Ok, that was my thinking as well. I think you might've misunderstood what I meant: this PR is setting up the frames in a such way that would not produce any parsing-time errors, so my question was whether such setup was desirable (I thought it was, and you confirmed it).
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @mjibson)
pkg/internal/sqlsmith/scalar.go, line 384 at r1 (raw file):
Previously, mjibson (Matt Jibson) wrote…
pass a
s *scopehere and uses.schema.rnd.Intninstead of therand.Intnuses.
Done.
pkg/internal/sqlsmith/scalar.go, line 387 at r1 (raw file):
Previously, mjibson (Matt Jibson) wrote…
Use the pattern similar to randDirection for this. You'll have to pass this func a
s *scopeargument. I guess BoundType is fine because of how you need to do the slicing for endBounds.
Done. Yeah, we need to be more careful with bounds in order to not produce frames that will result in a parsing-time error.
pkg/internal/sqlsmith/scalar.go, line 438 at r1 (raw file):
Previously, mjibson (Matt Jibson) wrote…
So this is where sqlsmith gets cool. Instead of hard coding an integer, use
makeScalar(s, types.Int, refs), and take arefs colRefsargument to this func. It'll produce some funky thing that spits out on int based on the currently available column names, or make some constants.
Done.
However I wonder if we can restrict the types of scalar expressions produced there. That OffsetExpr "must be an integer expression not containing any variables, aggregate functions, or window functions. The value must not be null or negative," and to the best of my understanding we need a non-negative non-null integer constant there.
There was a problem hiding this comment.
Reviewable status:
pkg/internal/sqlsmith/scalar.go, line 438 at r1 (raw file):
Previously, yuzefovich wrote…
Done.
However I wonder if we can restrict the types of scalar expressions produced there. That
OffsetExpr"must be an integer expression not containing any variables, aggregate functions, or window functions. The value must not be null or negative," and to the best of my understanding we need a non-negative non-null integer constant there.
Interesting. I looked over the code but didn't immediately see those requirements. Let's merge this as is and if it produces too many errors we can make it smarter.
I copied that quote in my previous response from Postgres docs since I was trying to emulate their behavior, and here is where the offsets are checked https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/sem/tree/type_check.go#L923. But I also agree that we can merge it as is and tweak it later if necessary, so thanks for the review! bors r+ |
36896: sqlsmith: add window frames r=yuzefovich a=yuzefovich Adds window frames to window functions. RANGE mode of framing is not supported with offset-type bounds. Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]>
Build succeeded |
Adds window frames to window functions. RANGE mode of framing is not
supported with offset-type bounds.
Release note: None