sql: fix conflicting udf options validation#85922
sql: fix conflicting udf options validation#85922craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
mgartner
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan)
pkg/sql/logictest/testdata/logic_test/udf line 25 at r1 (raw file):
statement error pq: conflicting or redundant options CREATE FUNCTION f() RETURNS INT IMMUTABLE LANGUAGE SQL LANGUAGE SQL AS $$ SELECT 1 $$;
nit: add test with CALLED ON NULL INPUT/STRICT/RETURNS NULL ON NULL INPUT too
pkg/sql/opt/optbuilder/create_function.go line 72 at r1 (raw file):
for _, option := range cf.Options { optTypeName := reflect.TypeOf(option).Name() if _, ok := options[optTypeName]; ok {
It's not a super performance critical code path, but the map and reflection are an anti-pattern. You could define a helper to check for duplicate options that simply stores a bool for each option type.
|
Previously, mgartner (Marcus Gartner) wrote…
ah yeah, I agree, I'll make a helper for it. thanks. |
b2e63c9 to
0e2dbbb
Compare
chengxiong-ruan
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/udf line 25 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit: add test with
CALLED ON NULL INPUT/STRICT/RETURNS NULL ON NULL INPUTtoo
Added more tests.
pkg/sql/opt/optbuilder/create_function.go line 72 at r1 (raw file):
Previously, chengxiong-ruan (Chengxiong Ruan) wrote…
ah yeah, I agree, I'll make a helper for it. thanks.
changed as suggested, thanks again!
f45a15c to
eaac80d
Compare
eaac80d to
7fbb264
Compare
7fbb264 to
45b9307
Compare
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 8 of 9 files at r2, all commit messages.
Reviewable status:
-- commits line 4 at r2:
Can you make this commit message more clear?
pkg/sql/sem/tree/udf_test.go line 76 at r2 (raw file):
for i, tc := range testCases { t.Run(strconv.Itoa(i+1), func(t *testing.T) {
nit: the options do know how to turn themselves into strings. It might make for better subtest names
This commit fixes a bug where function options can be conflicting with each other because we forgot to add option type names into the dedupe map. A helper is added to handle the validation. This commit also fixes another bug in ALTER FUNCTION where leakproof can be set for a non-immutable function. A helper is also added so the logic can be reused. Release note: None Release justification: Fixing a bug that function options can be conflicting with each other. Also Fixing a bug that leakproof can be set for non-immutable function with ALTER FUNCTION
45b9307 to
e0ce0fc
Compare
chengxiong-ruan
left a comment
There was a problem hiding this comment.
Reviewable status:
Previously, ajwerner wrote…
Can you make this commit message more clear?
oops... fixed.
pkg/sql/sem/tree/udf_test.go line 76 at r2 (raw file):
Previously, ajwerner wrote…
nit: the options do know how to turn themselves into strings. It might make for better subtest names
added test names by hand instead. thanks.
|
TFTR! 🙏 |
|
Build failed (retrying...): |
|
Build succeeded: |
A silly mistake that forgot to add option type to the map :(
Release note: None
Release justification: Fixing a bug that function options can be
conflicting with each other. Also Fixing a bug that leakproof can
be set for non-immutable function with ALTER FUNCTION