-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sqlsmith: add support for creating udfs #101205
sqlsmith: add support for creating udfs #101205
Conversation
90e1f53
to
b9daecf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving to unblock you from my previous review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball, @rharding6373, and @shermanCRL)
-- commits
line 6 at r1:
nit: I believe it's up to 9 since IntN(10)
returns numbers in the rang [0, 10)
.
pkg/internal/sqlsmith/relational.go
line 910 at r1 (raw file):
// 15%: FunctionCalledOnNullInput // 15%: FunctionReturnsNullOnNullInput // 15%: FunctionStrict
nit: Maybe this should be ~15%
- I was a bit confused why they didn't add up to 100%.
pkg/internal/sqlsmith/relational.go
line 924 at r1 (raw file):
// 15%: FunctionVolatile // 15%: FunctionImmutable // 15%: FunctionStable
nit: ~15%
pkg/internal/sqlsmith/relational.go
line 963 at r1 (raw file):
for i := 0; i < stmtCnt; i++ { // UDFs currently only support SELECT statements. stmt, _, ok := s.makeSelect(nil, nil)
nit: label these nils
pkg/internal/sqlsmith/relational.go
line 967 at r1 (raw file):
continue } stmts = append(stmts, stmt.String())
nit: I think we might have to use tree.AsStringWithFlags
with the tree.FmtParsable
flag to ensure these statements can be parsed.
pkg/internal/sqlsmith/relational.go
line 970 at r1 (raw file):
} if len(stmts) == 0 { return nil, false
UDFs with empty bodies are allowed. They're not that interesting but I think they're worth testing, at least some of the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @rharding6373 and @shermanCRL)
This PR gives sqlsmith the capability to issue `CREATE FUNCTION` commands. It randomly chooses from available and valid function options. It also contains up to 10 `SELECT` statements in the UDF body. This PR also leaves several TODOs, including support for params, return types other than records, and calling UDFs in queries after they are created. Epic: CRDB-20370 Informs: cockroachdb#90782 Release note: None
b9daecf
to
6c13a01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTRs!
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @DrewKimball, @jayshrivastava, @mgartner, and @shermanCRL)
Previously, mgartner (Marcus Gartner) wrote…
nit: I believe it's up to 9 since
IntN(10)
returns numbers in the rang[0, 10)
.
Good point. I increased the range to 11 to make this true.
pkg/internal/sqlsmith/relational.go
line 910 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit: Maybe this should be
~15%
- I was a bit confused why they didn't add up to 100%.
Changed to ~17%. Closer to 100, but now going over.
pkg/internal/sqlsmith/relational.go
line 924 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit: ~15%
Done.
pkg/internal/sqlsmith/relational.go
line 963 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit: label these nils
Done.
pkg/internal/sqlsmith/relational.go
line 967 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit: I think we might have to use
tree.AsStringWithFlags
with thetree.FmtParsable
flag to ensure these statements can be parsed.
Surprisingly didn't seem to be an issue when stressing TestGenerateParse
, but this seems like a good idea.
pkg/internal/sqlsmith/relational.go
line 970 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
UDFs with empty bodies are allowed. They're not that interesting but I think they're worth testing, at least some of the time.
Done.
pkg/ccl/changefeedccl/changefeed_test.go
line 0 at r1 (raw file):
Previously, jayshrivastava (Jayant) wrote…
Sounds good. This LGTM from the CDC side
Thank you for taking a look!
bors r+ |
Build succeeded: |
This PR gives sqlsmith the capability to issue
CREATE FUNCTION
commands. It randomly chooses from available and valid function options. It also contains up to 10SELECT
statements in the UDF body.This PR also leaves several TODOs, including support for params, return types other than records, and calling UDFs in queries after they are created.
Epic: CRDB-20370
Informs: #90782
Release note: None