Postgres: support for `OWNER TO` clause #1314

gainings · 2024-06-15T14:23:20Z

Support OWNER TO clause just for Postgres dialect
Closes #1293

…o Clause

iffyio · 2024-06-20T20:18:10Z

src/ast/ddl.rs

+
+    /// `OWNER TO { <new_owner> | CURRENT_ROLE | CURRENT_USER | SESSION_USER }`
+    ///
+    /// Note: this is a PostgreSQL-specific operation.


Could we include a link to the docs for the feature? I think it'd be this one https://www.postgresql.org/docs/current/sql-altertable.html

done in a3a4996

iffyio · 2024-06-20T20:21:47Z

src/ast/ddl.rs

+pub enum Owner {
+    Ident(Ident),
+    Expr(Expr),
+}


Suggested change

pub enum Owner {

Ident(Ident),

Expr(Expr),

}

pub enum Owner {

Ident(Ident),

CurrentRole,

CurrentUser

SessionUser

}

I'm thinking the Expr might be somewhat opaque to represent the others, from the docs the other variants seem enumerable so figured we can represent them verbatim in the enum as above?

@iffyio Thanks for the review!

Currently CURRENT_USER, and SESSION_USER are supported as Expr::Function like this.

2024-06-22T13:04:10.639Z DEBUG [sqlparser::parser] Parsing sql 'SELECT CURRENT_USER, CURRENT_ROLE, SESSION_USER; '... 2024-06-22T13:04:10.639Z DEBUG [sqlparser::parser] parsing expr 2024-06-22T13:04:10.639Z DEBUG [sqlparser::parser] prefix: Function(Function { name: ObjectName([Ident { value: "CURRENT_USER", quote_style: None }]), args: None, filter: None, null_treatment: None, over: None, within_group: [] }) 2024-06-22T13:04:10.639Z DEBUG [sqlparser::parser] get_next_precedence() TokenWithLocation { token: Comma, location: Location { line: 1, column: 20 } } 2024-06-22T13:04:10.639Z DEBUG [sqlparser::parser] 0: , 1: CURRENT_ROLE 2: , 2024-06-22T13:04:10.639Z DEBUG [sqlparser::parser] next precedence: 0 2024-06-22T13:04:10.639Z DEBUG [sqlparser::parser] parsing expr 2024-06-22T13:04:10.639Z DEBUG [sqlparser::parser] prefix: Identifier(Ident { value: "CURRENT_ROLE", quote_style: None }) 2024-06-22T13:04:10.639Z DEBUG [sqlparser::parser] get_next_precedence() TokenWithLocation { token: Comma, location: Location { line: 1, column: 34 } } 2024-06-22T13:04:10.639Z DEBUG [sqlparser::parser] 0: , 1: SESSION_USER 2: ; 2024-06-22T13:04:10.639Z DEBUG [sqlparser::parser] next precedence: 0 2024-06-22T13:04:10.639Z DEBUG [sqlparser::parser] parsing expr 2024-06-22T13:04:10.639Z DEBUG [sqlparser::parser] prefix: Function(Function { name: ObjectName([Ident { value: "SESSION_USER", quote_style: None }]), args: None, filter: None, null_treatment: None, over: None, within_group: [] }) 2024-06-22T13:04:10.639Z DEBUG [sqlparser::parser] get_next_precedence() TokenWithLocation { token: SemiColon, location: Location { line: 1, column: 48 } } 2024-06-22T13:04:10.639Z DEBUG [sqlparser::parser] 0: ; 1: EOF 2: EOF 2024-06-22T13:04:10.639Z DEBUG [sqlparser::parser] next precedence: 0

https://github.com/sqlparser-rs/sqlparser-rs/blob/f16c1afed0fa273228e74a633f3885c9c6609911/src/parser/mod.rs#L998-L1010

I think it would be better to maintain this compatibility.

Additionally, CURRENT_ROLE must also be considered.

Ah I see, I think it makes a lot of sense to represent them as functions when parsing arbitrary expressions - when parsing an Owner however, spontaneously doesn't feel the same since the context is much more restricted. Thinking here we can be explicit about what comes out of the parser which would be desirable from an API pov.

I agree it is better to be explicit and match the allowed postgres syntax more exactly. I double checked https://www.postgresql.org/docs/current/sql-altertable.html and it says:

OWNER TO { new_owner | CURRENT_ROLE | CURRENT_USER | SESSION_USER }

...
new_owner
The user name of the new owner of the table.

So I think @iffyio 's suggestion is a good one.

I took the liberty of making this change in 0ff17d7 as I am preparing for a sqlparser release

iffyio · 2024-06-20T20:22:33Z

src/parser/mod.rs

@@ -6274,6 +6274,30 @@ impl<'a> Parser<'a> {
            self.expect_keyword(Keyword::WITH)?;
            let table_name = self.parse_object_name(false)?;
            AlterTableOperation::SwapWith { table_name }
+        } else if dialect_of!(self is PostgreSqlDialect)


Suggested change

} else if dialect_of!(self is PostgreSqlDialect)

} else if dialect_of!(self is PostgreSqlDialect | GenericDialect)

if no conflicts we can include support for the generic dialect

Done in 25d6a40

iffyio · 2024-06-20T20:33:04Z

src/parser/mod.rs

+        {
+            let next_token = self.next_token();
+            let new_owner = match next_token.token {
+                Token::DoubleQuotedString(ref s) => Owner::Ident(Ident::new(s.to_string())),


instead of having two separate cases (this DoubleQuotedString and NoKeyword below) to handle the ident variant, would it make sense to call parse_identifier if none of the other variants match?
so roughly

match self.parse_one_of_keywords(&[CURRENT_USER, CURRENT_ROLE, SESSION_USER]) { Some(CURRENT_USER) => {} ... _ =|> self.parse_identifier() }

in 0ff17d7

coveralls · 2024-07-08T10:36:32Z

Pull Request Test Coverage Report for Build 9838925240

Details

54 of 58 (93.1%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.007%) to 89.18%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser/mod.rs	13	14	92.86%
tests/sqlparser_postgres.rs	33	36	91.67%

Totals
Change from base Build 9838850938:	0.007%
Covered Lines:	26639
Relevant Lines:	29871

💛 - Coveralls

…ble-owner-to

alamb · 2024-07-08T11:26:17Z

src/parser/mod.rs

+                    match self.parse_identifier(false) {
+                        Ok(ident) => Owner::Ident(ident),
+                        Err(e) => {
+                            return Err(ParserError::ParserError(format!("Expected CURRENT_USER, CURRENT_ROLE, SESSION_USER or identifier after OWNER TO. {e}")))


Making sure the error message had some additional context was the only thing that I found awkward. Otherwise I think @iffyio 's suggestions made the code easier to understand and easier to use overall

alamb

@gainings and @iffyio if you have a moment to review the changes I made to this PR I would be most appreciative. I am hoping to get a sqlparser-rs release out tomorrow: #1296

…ble-owner-to

iffyio

Changes LGTM!

alamb · 2024-07-09T11:49:54Z

Thanks again @gainings and @iffyio

gainings · 2024-07-09T15:21:59Z

Thank you for @alamb and @iffyio
And I apologize for the delayed response.

alamb · 2024-07-09T20:53:31Z

No worries -- thank you for the contribution @gainings and I apologize for my late review / update.

gainings added 6 commits June 15, 2024 23:10

Add keyword for postgres

af560ee

Add OwnerTo ddl ast

c351eb8

Add owner to parser in alter talbe operation

e4d1e95

Add postgres test for owner to

1d5f01d

Parse OWNER TO clause only for Postgresql

c56120e

Support CURRENT_ROLE, CURRENT_USER, SESSION_USER Functions for OwnerT…

5c21442

…o Clause

gainings changed the title ~~BigQuery: support for OWNER TO clause~~ Postgres: support for OWNER TO clause Jun 15, 2024

gainings marked this pull request as ready for review June 15, 2024 14:24

gainings added 3 commits June 17, 2024 20:41

Fix expected error message for OwnerTo

4d06a61

Fix postgres test to support CURRENT_USER, CURRENT_ROLE, SESSION_USER

52c162d

Apply cargo fmt

6076c2d

gainings force-pushed the postgres-add-alter-table-owner-to branch from 3edc55b to 6076c2d Compare June 17, 2024 11:42

iffyio reviewed Jun 20, 2024

View reviewed changes

alamb added 4 commits July 8, 2024 06:49

Add link

a3a4996

Support generic dialect

25d6a40

Use explicit variants

0ff17d7

Merge remote-tracking branch 'origin/main' into postgres-add-alter-ta…

4180911

…ble-owner-to

alamb reviewed Jul 8, 2024

View reviewed changes

alamb approved these changes Jul 8, 2024

View reviewed changes

alamb added 2 commits July 8, 2024 07:37

Merge remote-tracking branch 'origin/main' into postgres-add-alter-ta…

210c5c3

…ble-owner-to

Update for latest error messages

4340c65

iffyio approved these changes Jul 8, 2024

View reviewed changes

alamb merged commit 32b8276 into apache:main Jul 9, 2024
10 checks passed

Jesse-Bakker mentioned this pull request Jul 29, 2024

Parse ClickHouse table-valued function SETTINGS clause langdb/sqlparser-rs#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Postgres: support for `OWNER TO` clause #1314

Postgres: support for `OWNER TO` clause #1314

gainings commented Jun 15, 2024 •

edited

Loading

iffyio Jun 20, 2024

alamb Jul 8, 2024

iffyio Jun 20, 2024

gainings Jun 22, 2024

iffyio Jun 23, 2024

alamb Jul 8, 2024

iffyio Jun 20, 2024

alamb Jul 8, 2024

iffyio Jun 20, 2024

alamb Jul 8, 2024

coveralls commented Jul 8, 2024 •

edited

Loading

alamb Jul 8, 2024

alamb left a comment

iffyio left a comment

alamb commented Jul 9, 2024

gainings commented Jul 9, 2024

alamb commented Jul 9, 2024

	} else if dialect_of!(self is PostgreSqlDialect)
	} else if dialect_of!(self is PostgreSqlDialect \| GenericDialect)

Postgres: support for OWNER TO clause #1314

Postgres: support for OWNER TO clause #1314

Conversation

gainings commented Jun 15, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 8, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9838925240

Details

💛 - Coveralls

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

iffyio left a comment

Choose a reason for hiding this comment

alamb commented Jul 9, 2024

gainings commented Jul 9, 2024

alamb commented Jul 9, 2024

Postgres: support for `OWNER TO` clause #1314

Postgres: support for `OWNER TO` clause #1314

gainings commented Jun 15, 2024 •

edited

Loading

coveralls commented Jul 8, 2024 •

edited

Loading