-
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
sql: implement oidvectortypes builtin #108467
Conversation
9bca0f2
to
99cb5a1
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.
nice! just have small comments
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)
pkg/sql/sem/builtins/builtins.go
line 4012 at r1 (raw file):
oidDatum := datum.(*tree.DOid) var typ *types.T if resolvedTyp, ok := types.OidToType[oidDatum.Oid]; ok {
does ResolveTypeByOID
not already have this logic? not a huge deal, but just curious
pkg/sql/sem/builtins/builtins.go
line 4020 at r1 (raw file):
} } result.WriteString(typ.SQLString())
i believe we want typ.Name()
. compare with postgres to see which format it uses.
pkg/sql/sem/builtins/builtins.go
line 4027 at r1 (raw file):
return tree.NewDString(result.String()), nil }, Info: "Generates a comma seperated string of type names from an oidvector",
nit: period at the end
pkg/sql/logictest/testdata/logic_test/builtin_function
line 4042 at r1 (raw file):
query T rowsort SELECT oidvectortypes(proargtypes) FROM pg_proc WHERE oid=263
could you add a test for a UDF that has user-defined types as arg types?
99cb5a1
to
c6906f8
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)
pkg/sql/sem/builtins/builtins.go
line 4012 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
does
ResolveTypeByOID
not already have this logic? not a huge deal, but just curious
Sadly, it doesn't. I started off just calling that, but then builtin types didn't work :/
pkg/sql/sem/builtins/builtins.go
line 4020 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i believe we want
typ.Name()
. compare with postgres to see which format it uses.
Done.
pkg/sql/logictest/testdata/logic_test/builtin_function
line 4042 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
could you add a test for a UDF that has user-defined types as arg types?
Done.
0075509
to
b2c55b4
Compare
Previously, the oidvectortypes builtin in wasn't implemented, causing a compatibility gap for tools that need to format oidvectors. To address this, this patch adds the oidvectortypes built in. Fixes: cockroachdb#107942 Release note (sql change): The oidvectortypes built-in has been implemented, which can format oidvector.
b2c55b4
to
a24025d
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.
lgtm!
Reviewable status: complete! 0 of 0 LGTMs obtained
@rafiss TFTR! bors r+ |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from a24025d to blathers/backport-release-23.1-108467: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Build succeeded: |
Previously, the oidvectortypes builtin in wasn't implemented, causing a compatibility gap for tools
that need to format oidvectors. To address this, this patch adds the oidvectortypes built in.
Fixes: #107942
Release note (sql change): The oidvectortypes built-in has been implemented, which can format oidvector.