Support redshift's columns definition list for system information functions #769

mskrzypkows · 2022-12-23T13:18:16Z

pg_get_late_binding_view_cols
pg_get_cols
pg_get_grantee_by_iam_role
pg_get_iam_role_by_user

pg_get_late_binding_view_cols pg_get_cols pg_get_grantee_by_iam_role pg_get_iam_role_by_user

coveralls · 2022-12-23T13:21:27Z

Pull Request Test Coverage Report for Build 4206736323

109 of 111 (98.2%) changed or added relevant lines in 12 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.09%) to 86.142%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser.rs	25	26	96.15%
tests/sqlparser_redshift.rs	42	43	97.67%

Totals
Change from base Build 4206701096:	0.09%
Covered Lines:	13371
Relevant Lines:	15522

💛 - Coveralls

alamb

Thank you for the contribution @mskrzypkows !

alamb · 2022-12-28T13:50:48Z

src/ast/mod.rs

+/// when used with redshift pg_get_late_binding_view_cols/pg_get_cols)
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+pub struct ColsDefinition {


Is there any reason not to use the existing ColumnDef?

https://github.com/sqlparser-rs/sqlparser-rs/blob/61c661c234518ae02ae2c50d317e4640e4b90e26/src/ast/ddl.rs#L479-L486

I think that would make this PR significantly shorter as well as supporting other options

What do you think?

I think that the name is misleading. I should have named it e.g. TableAliasDefinition, because it defines what will be a result table, what column names it will have.
Like in the example:

select * from pg_get_late_binding_view_cols() cols(view_schema name, view_name name, col_name name, col_type varchar, col_num int); view_schema | view_name | col_name | col_type | col_num ------------+-----------+------------+-----------------------------+-------- public | sales_lbv | salesid | integer | 1 public | sales_lbv | listid | integer | 2 ...

But I think it's a good idea to use ColumnDef inside the vector, instead of vector of IdentPairs.

What's your opinion?

I think that the name is misleading. I should have named it e.g. TableAliasDefinition, because it defines what will be a result table, what column names it will have.

Sounds like a good idea to me

But I think it's a good idea to use ColumnDef inside the vector, instead of vector of IdentPairs.

👍

I've tried using ColumnDef and failed because name is not a column type. It's strange, redshift doesn't define it as a keyword nor a type, but it's used in such a case. I think it's not a good idea to add it as additional type, so I'll rather back to IdentPair, or is there a better way to handle it?

So I looked at the docs for

https://docs.aws.amazon.com/redshift/latest/dg/PG_GET_GRANTEE_BY_IAMROLE.html

I see in the example it has something like

select grantee, grantee_type, cmd_type FROM pg_get_grantee_by_iam_role('arn:aws:iam::123456789012:role/Redshift-S3-Write') res_grantee(grantee text, grantee_type text, cmd_type text) ORDER BY 1,2,3;

I think this PR is related to this bit of the query (note there is no comma between the call to pg_get_grantee_by_iam_role)

res_grantee(grantee text, grantee_type text, cmd_type text)

I have several confusions:

Are you sure this construct is specific to the system information functions, or is a more general mechanism (it look like a more general mechanism that expands out a tuple to a table to me)

If it is a view definition, I would expect it to be parsed as ident, variable type not as just generic idents

Maybe someone could do some more research into what exactly this syntax construct in Redshift is called and what its rules are

You're right, it looks like a more general mechanism. I've checked the documentation, but haven't found any general syntax description for it.

So if we have an additional name type, then it has to be added to the DataType, right?

AugustoFKL · 2023-01-05T02:34:04Z

src/parser.rs

+        &mut self,
+        name: &ObjectName,
+    ) -> Result<Option<ColsDefinition>, ParserError> {
+        if !dialect_of!(self is RedshiftSqlDialect) {


@mskrzypkows I guess this could consider the GenericDialect as well.

These functions are specific for the Redshift: https://docs.aws.amazon.com/redshift/latest/dg/r_System_information_functions.html so I think it's not needed for GenericDialect.

I know, but the Generic dialect is supposed to be the most permissive one. Unless there are syntax conflicts (e.g., accepting those would break another part of the code), I can't see why not.

AugustoFKL · 2023-01-05T02:36:21Z

src/parser.rs

+            .value
+            .to_lowercase();
+
+        if fname == "pg_get_late_binding_view_cols"


@alamb isn't this too much related to semantic logic? it isn't just a database metadata function? Is it really necessary to have that deep information?

Yes I agree -- I would not expect this list to be hard coded. Instead I would expect any identifier to be accepted here and then the downstream crate would check against a list if it wanted.

This design goal is explained in https://github.com/sqlparser-rs/sqlparser-rs#syntax-vs-semantics

…hift_special_system_information_functions

added generic dialect

AugustoFKL

@mskrzypkows there are some actions failing, could you please take a look?

alamb · 2023-01-09T22:02:13Z

I am sorry I am a bit behind on reviews / merging in sqlparser-rs. I hope to be able to catch up later this week

mskrzypkows · 2023-01-10T09:07:11Z

@mskrzypkows there are some actions failing, could you please take a look?

Sure, I'll fix it.

…hift_special_system_information_functions

parentheses instead of function name

alamb

I think this looks good to me now. Thank you @mskrzypkows

This is one of the more unique syntaxes I have seen in SQL 🤯

…l_system_information_functions

alamb · 2023-02-17T19:01:41Z

I took the liberty of merging up from main and fixing a (new) clippy lint on this PR

…tion functions (apache#769)" This reverts commit c35dcc9.

alamb · 2023-03-06T14:50:58Z

This change seems to have caused a regression in 0.31.0 -- see #826. Here is a PR to back it out: #827

Looking more at the example https://docs.aws.amazon.com/redshift/latest/dg/PG_GET_GRANTEE_BY_IAMROLE.html

select grantee, grantee_type, cmd_type 
FROM 
  pg_get_grantee_by_iam_role('arn:aws:iam::123456789012:role/Redshift-S3-Write') 
  res_grantee(grantee text, grantee_type text, cmd_type text) 
ORDER BY
 1,2,3;

Rather than a special table definition syntax I think this is actually

FROM <table_function> <table_alias>(<col_alias1>, <col_alias2>, ...)

So

select grantee, grantee_type, cmd_type 
FROM 
  pg_get_grantee_by_iam_role('arn:aws:iam::123456789012:role/Redshift-S3-Write') <-- this is "table function"
  res_grantee(grantee text, grantee_type text, cmd_type text) <-- `res_grantee` is the table alias, and the other things are column aliases
ORDER BY
 1,2,3;

I see perhaps the sqlparser column alias syntax doesn't support datatypes, maybe that could be extended

…column definition list (#827) * Fix table alias parsing regression * Revert "Support redshift's columns definition list for system information functions (#769)" This reverts commit c35dcc9.

parsing of redshift's column definition list for

db9df86

pg_get_late_binding_view_cols pg_get_cols pg_get_grantee_by_iam_role pg_get_iam_role_by_user

mskrzypkows changed the title ~~parsing of redshift's column definition list for~~ Parsing of redshift's columns definition list for system information functions Dec 23, 2022

alamb changed the title ~~Parsing of redshift's columns definition list for system information functions~~ Support redshift's columns definition list for system information functions Dec 28, 2022

alamb reviewed Dec 28, 2022

View reviewed changes

AugustoFKL reviewed Jan 5, 2023

View reviewed changes

Maciej Skrzypkowski added 2 commits January 9, 2023 12:04

Merge branch 'main' of github.com:sqlparser-rs/sqlparser-rs into reds…

158afdb

…hift_special_system_information_functions

Renamed ColsDefinition to TableAliasDefinition

fec98ef

added generic dialect

AugustoFKL reviewed Jan 9, 2023

View reviewed changes

Maciej Skrzypkowski added 5 commits January 10, 2023 21:43

Tests fixed

d81d446

Visitor for IdentPair

17e11a1

Merge branch 'main' of github.com:sqlparser-rs/sqlparser-rs into reds…

c653872

…hift_special_system_information_functions

Merge branch 'main' of github.com:sqlparser-rs/sqlparser-rs into reds…

8d2ce41

…hift_special_system_information_functions

Parsing redshift table alias based on indentifier and

4f570a7

parentheses instead of function name

alamb approved these changes Feb 17, 2023

View reviewed changes

alamb added 2 commits February 17, 2023 13:59

Merge remote-tracking branch 'sqlparser-rs/main' into redshift_specia…

6353d8c

…l_system_information_functions

fix clippy

4d42aa1

alamb merged commit c35dcc9 into apache:main Feb 17, 2023

alamb mentioned this pull request Mar 6, 2023

regression parsing table aliases in 0.31.0 #826

Closed

alamb added a commit to alamb/sqlparser-rs that referenced this pull request Mar 6, 2023

Revert "Support redshift's columns definition list for system informa…

1f7ecc9

…tion functions (apache#769)" This reverts commit c35dcc9.

alamb added a commit to alamb/sqlparser-rs that referenced this pull request Mar 6, 2023

Revert "Support redshift's columns definition list for system informa…

af55e9c

…tion functions (apache#769)" This reverts commit c35dcc9.

alamb mentioned this pull request Mar 6, 2023

Fix table alias parsing regression in 0.31.0 by backing out redshift column definition list #827

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support redshift's columns definition list for system information functions #769

Support redshift's columns definition list for system information functions #769

mskrzypkows commented Dec 23, 2022

coveralls commented Dec 23, 2022 •

edited

Loading

alamb left a comment

alamb Dec 28, 2022

mskrzypkows Jan 2, 2023

alamb Jan 2, 2023

mskrzypkows Jan 3, 2023

alamb Jan 15, 2023 •

edited

Loading

mskrzypkows Jan 20, 2023

AugustoFKL Jan 5, 2023

mskrzypkows Jan 5, 2023

AugustoFKL Jan 7, 2023

AugustoFKL Jan 5, 2023

alamb Jan 15, 2023

AugustoFKL left a comment

alamb commented Jan 9, 2023

mskrzypkows commented Jan 10, 2023

alamb left a comment

alamb commented Feb 17, 2023

alamb commented Mar 6, 2023

Support redshift's columns definition list for system information functions #769

Support redshift's columns definition list for system information functions #769

Conversation

mskrzypkows commented Dec 23, 2022

coveralls commented Dec 23, 2022 • edited Loading

Pull Request Test Coverage Report for Build 4206736323

💛 - Coveralls

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb Jan 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AugustoFKL left a comment

Choose a reason for hiding this comment

alamb commented Jan 9, 2023

mskrzypkows commented Jan 10, 2023

alamb left a comment

Choose a reason for hiding this comment

alamb commented Feb 17, 2023

alamb commented Mar 6, 2023

coveralls commented Dec 23, 2022 •

edited

Loading

alamb Jan 15, 2023 •

edited

Loading