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: populate information_schema.user_defined_types and attributes #111401
Conversation
ideally, i get #111179 merged so that i can write tests and check them locally properly |
e1f2759
to
86f63c6
Compare
86f63c6
to
2332326
Compare
8ed027a
to
8fa219c
Compare
72f22d5
to
315520c
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.
this is great! my comments are all minor
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom)
pkg/sql/sem/builtins/pg_builtins.go
line 2186 at r1 (raw file):
), // NOTE: this could be defined as a user-defined function, like
nit: this comment seems outdated, since that's how the function is defined now. we could just keep the part that links to postgres source
pkg/sql/sem/builtins/pg_builtins.go
line 2203 at r1 (raw file):
// ELSE null // END; "information_schema._pg_char_octet_length": makeBuiltin(tree.FunctionProperties{Category: builtinconstants.CategorySystemInfo},
this function could use some basic tests
pkg/sql/sem/builtins/pg_builtins.go
line 2212 at r1 (raw file):
Body: `SELECT CASE WHEN $1 IN (25, 1042, 1043) THEN CASE WHEN $2 = -1
nit: the comments that are included above would make this more understandable. i mean the /* text, char, varchar */
and /* default typmod */
comments
pkg/sql/sem/builtins/pg_builtins.go
line 2273 at r3 (raw file):
), // NOTE: this could be defined as a user-defined function, like
nit: "could be" is outdated here; this comment just needs a link to the postgres code
pkg/sql/sem/builtins/pg_builtins.go
line 2294 at r3 (raw file):
ReturnType: tree.FixedReturnType(types.String), Body: `SELECT CASE WHEN $1 IN (1186)
nit: the /* interval */
comment could be helpful here
pkg/sql/logictest/testdata/logic_test/pg_builtins
line 951 at r3 (raw file):
ORDER BY a.attname ---- attname typname information_schema._pg_interval_type
nit: could you add some test cases where the result of `pg_interval_type is not null?
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/pg_builtins.go
line 2186 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: this comment seems outdated, since that's how the function is defined now. we could just keep the part that links to postgres source
done
pkg/sql/sem/builtins/pg_builtins.go
line 2203 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
this function could use some basic tests
these were added in the commit with pg_encoding_max_length - since pg_encoding_max_length was needed for _pg_char_octet_length
i added some tests for pg_encoding_max_length and moved the commits next to eachother!
pkg/sql/sem/builtins/pg_builtins.go
line 2212 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: the comments that are included above would make this more understandable. i mean the
/* text, char, varchar */
and/* default typmod */
comments
done
pkg/sql/sem/builtins/pg_builtins.go
line 2273 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: "could be" is outdated here; this comment just needs a link to the postgres code
done
pkg/sql/sem/builtins/pg_builtins.go
line 2294 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: the
/* interval */
comment could be helpful here
done
pkg/sql/logictest/testdata/logic_test/pg_builtins
line 951 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: could you add some test cases where the result of `pg_interval_type is not null?
hmmm looks like this will return null for now due to how interval types in TypeModifier get handled - let me get a fix up for this. think I need to have another check for if t.InternalType.IntervalDurationField.DurationType != IntervalDurationType_UNSET - not sure yet what I should be returning. maybe postgres source code has something for me
315520c
to
12bbf3b
Compare
This is now looking like a separate issue - as this looks to be a compatibility bug that existed before this PR (see issue above) For now, information_schema._pg_interval_type does not work properly on intervals that have durations on them |
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!! nice work on seeing this through. (feel free to rebase and merge)
Reviewable status: complete! 0 of 0 LGTMs obtained
This commit adds an implementation for the `information_schema._pg_char_octet_length` builtin. The builtin is implemented as a user-defined function in Postgres [here](https://github.com/postgres/postgres/blob/master/src/backend/catalog/information_schema.sql) Needed for: cockroachdb#109603 Epic: none Release note (sql change): The `information_schema._pg_char_octet_length` builtin function is now supported, which improves compatibility with PostgreSQL.
This commit adds an implementation for the `pg_encoding_max_length` builtin. Needed for: cockroachdb#109603 Epic: none Release note (sql change): The `pg_encoding_max_length` builtin function is now supported, which improves compatibility with PostgreSQL.
This commit adds an implementation for the `information_schema._pg_datetime_precision` builtin. The builtin is implemented as a user-defined function in Postgres [here](https://github.com/postgres/postgres/blob/master/src/backend/catalog/information_schema.sql) Needed for: cockroachdb#109603 Epic: none Release note (sql change): The `information_schema._pg_datetime_precision` builtin function is now supported, which improves compatibility with PostgreSQL.
This commit adds an implementation for the `information_schema._pg_interval_type` builtin. The builtin is implemented as a user-defined function in Postgres [here](https://github.com/postgres/postgres/blob/master/src/backend/catalog/information_schema.sql) Needed for: cockroachdb#109603 Epic: none Release note (sql change): The `information_schema._pg_interval_type` builtin function is now supported, which improves compatibility with PostgreSQL.
12bbf3b
to
774c451
Compare
TFTR! ('-')7 bors r=rafiss |
bors r- |
Canceled. |
This patch populates information_schema.user_defined_types with information about user defined types and information_schema.attributes with information about the attributes of composite data types. Epic: none Fixes: cockroachdb#109603 Release note (sql change): This patch populates information_schema.user_defined_types with information about user defined types and information_schema.attributes with information about the attributes of composite data types.
774c451
to
391f96b
Compare
bors r+ |
Build succeeded: |
commit 1-4:
sql: add necessary builtins for information_schema.attributes
These patches add the following builtins:
Needed for: #109603
Epic: none
Release note (sql change): The information_schema._pg_char_octet_length, information_schema._pg_datetime_precision, information_schema._pg_interval_type, and pg_encoding_max_length builtin functions are now supported, which improves compatibility with PostgreSQL.
last commit:
sql: populate information_schema.user_defined_types and attributes
This patch populates information_schema.user_defined_types with
information about user defined types and information_schema.attributes
with information about the attributes of composite data types.
Epic: none
Fixes: #109603
Release note (sql change): This patch populates
information_schema.user_defined_types with information about user
defined types and information_schema.attributes with information about
the attributes of composite data types.