Skip to content
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

Postgres: support for OWNER TO clause #1314

Merged
merged 15 commits into from
Jul 9, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/ast/ddl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,28 @@ pub enum AlterTableOperation {
SwapWith { table_name: ObjectName },
/// 'SET TBLPROPERTIES ( { property_key [ = ] property_val } [, ...] )'
SetTblProperties { table_properties: Vec<SqlOption> },

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in a3a4996

OwnerTo { new_owner: Owner },
}

#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
pub enum Owner {
Ident(Ident),
Expr(Expr),
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


impl fmt::Display for Owner {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
Owner::Ident(ident) => write!(f, "{}", ident),
Owner::Expr(expr) => write!(f, "{}", expr),
}
}
}

#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
Expand Down Expand Up @@ -322,6 +344,9 @@ impl fmt::Display for AlterTableOperation {
AlterTableOperation::SwapWith { table_name } => {
write!(f, "SWAP WITH {table_name}")
}
AlterTableOperation::OwnerTo { new_owner } => {
write!(f, "OWNER TO {new_owner}")
}
AlterTableOperation::SetTblProperties { table_properties } => {
write!(
f,
Expand Down
4 changes: 2 additions & 2 deletions src/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ pub use self::dcl::{AlterRoleOperation, ResetConfig, RoleOption, SetConfigValue}
pub use self::ddl::{
AlterColumnOperation, AlterIndexOperation, AlterTableOperation, ColumnDef, ColumnOption,
ColumnOptionDef, ConstraintCharacteristics, DeferrableInitial, GeneratedAs,
GeneratedExpressionMode, IndexOption, IndexType, KeyOrIndexDisplay, Partition, ProcedureParam,
ReferentialAction, TableConstraint, UserDefinedTypeCompositeAttributeDef,
GeneratedExpressionMode, IndexOption, IndexType, KeyOrIndexDisplay, Owner, Partition,
ProcedureParam, ReferentialAction, TableConstraint, UserDefinedTypeCompositeAttributeDef,
UserDefinedTypeRepresentation, ViewColumnDef,
};
pub use self::dml::{Delete, Insert};
Expand Down
1 change: 1 addition & 0 deletions src/keywords.rs
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,7 @@ define_keywords!(
OVERLAY,
OVERWRITE,
OWNED,
OWNER,
PARALLEL,
PARAMETER,
PARQUET,
Expand Down
24 changes: 24 additions & 0 deletions src/parser/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 25d6a40

&& self.parse_keywords(&[Keyword::OWNER, Keyword::TO])
{
let next_token = self.next_token();
let new_owner = match next_token.token {
Token::DoubleQuotedString(ref s) => Owner::Ident(Ident::new(s.to_string())),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in 0ff17d7

Token::Word(ref w) => match w.keyword {
Keyword::CURRENT_USER | Keyword::CURRENT_ROLE | Keyword::SESSION_USER => {
Owner::Expr(Expr::Function(Function {
name: ObjectName(vec![w.to_ident()]),
args: FunctionArguments::None,
null_treatment: None,
filter: None,
over: None,
within_group: vec![],
}))
},
Keyword::NoKeyword => Owner::Ident(w.to_ident()),
_ => self.expected("CURRENT_USER, CURRENT_ROLE, SESSION_USER or identifier after OWNER TO clause", next_token)?,
},
_ => self.expected("Token::Word", next_token)?
};

AlterTableOperation::OwnerTo { new_owner }
} else {
let options: Vec<SqlOption> =
self.parse_options_with_keywords(&[Keyword::SET, Keyword::TBLPROPERTIES])?;
Expand Down
79 changes: 79 additions & 0 deletions tests/sqlparser_postgres.rs
Original file line number Diff line number Diff line change
Expand Up @@ -713,6 +713,85 @@ fn parse_alter_table_add_columns() {
}
}

#[test]
fn parse_alter_table_owner_to() {
struct TestCase {
sql: &'static str,
expected_owner: Owner,
}

let test_cases = vec![
TestCase {
sql: "ALTER TABLE tab OWNER TO new_owner",
expected_owner: Owner::Ident(Ident::new("new_owner".to_string())),
},
TestCase {
sql: "ALTER TABLE tab OWNER TO \"new_owner\"",
expected_owner: Owner::Ident(Ident::with_quote('\"', "new_owner".to_string())),
},
TestCase {
sql: "ALTER TABLE tab OWNER TO CURRENT_USER",
expected_owner: Owner::Expr(Expr::Function(Function {
name: ObjectName(vec![Ident::new("CURRENT_USER")]),
args: FunctionArguments::None,
null_treatment: None,
filter: None,
over: None,
within_group: vec![],
})),
},
TestCase {
sql: "ALTER TABLE tab OWNER TO CURRENT_ROLE",
expected_owner: Owner::Expr(Expr::Function(Function {
name: ObjectName(vec![Ident::new("CURRENT_ROLE")]),
args: FunctionArguments::None,
null_treatment: None,
filter: None,
over: None,
within_group: vec![],
})),
},
TestCase {
sql: "ALTER TABLE tab OWNER TO SESSION_USER",
expected_owner: Owner::Expr(Expr::Function(Function {
name: ObjectName(vec![Ident::new("SESSION_USER")]),
args: FunctionArguments::None,
null_treatment: None,
filter: None,
over: None,
within_group: vec![],
})),
},
];

for case in test_cases {
match pg().verified_stmt(case.sql) {
Statement::AlterTable {
name,
if_exists: _,
only: _,
operations,
location: _,
} => {
assert_eq!(name.to_string(), "tab");
assert_eq!(
operations,
vec![AlterTableOperation::OwnerTo {
new_owner: case.expected_owner.clone()
}]
);
}
_ => unreachable!("Expected an AlterTable statement"),
}
}

let res = pg().parse_sql_statements("ALTER TABLE tab OWNER TO CREATE");
assert_eq!(
ParserError::ParserError("Expected CURRENT_USER, CURRENT_ROLE, SESSION_USER or identifier after OWNER TO clause, found: CREATE".to_string()),
res.unwrap_err()
);
}

#[test]
fn parse_create_table_if_not_exists() {
let sql = "CREATE TABLE IF NOT EXISTS uk_cities ()";
Expand Down
Loading