Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Feb 17, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
db9df86
parsing of redshift's column definition list for
Dec 23, 2022
158afdb
Merge branch 'main' of github.com:sqlparser-rs/sqlparser-rs into reds…
Jan 9, 2023
fec98ef
Renamed ColsDefinition to TableAliasDefinition
Jan 9, 2023
d81d446
Tests fixed
Jan 10, 2023
17e11a1
Visitor for IdentPair
Jan 11, 2023
c653872
Merge branch 'main' of github.com:sqlparser-rs/sqlparser-rs into reds…
Jan 11, 2023
8d2ce41
Merge branch 'main' of github.com:sqlparser-rs/sqlparser-rs into reds…
Jan 20, 2023
4f570a7
Parsing redshift table alias based on indentifier and
Jan 20, 2023
6353d8c
Merge remote-tracking branch 'sqlparser-rs/main' into redshift_specia…
alamb Feb 17, 2023
4d42aa1
fix clippy
alamb Feb 17, 2023
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
27 changes: 27 additions & 0 deletions src/ast/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3954,6 +3954,33 @@ impl fmt::Display for SearchModifier {
}
}

/// A cols definition (i.e. `cols(view_schema name, view_name name, col_name name, col_type varchar, col_num int)`
/// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@alamb alamb Jan 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. 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)
  2. 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. 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.
  2. So if we have an additional name type, then it has to be added to the DataType, right?

pub name: Ident,
pub args: Vec<IdentPair>,
}

impl fmt::Display for ColsDefinition {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "{}({})", self.name, display_comma_separated(&self.args))?;
Ok(())
}
}

#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
pub struct IdentPair(pub Ident, pub Ident);

impl fmt::Display for IdentPair {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "{} {}", self.0, self.1)?;
Ok(())
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
7 changes: 7 additions & 0 deletions src/ast/query.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,9 @@ pub enum TableFactor {
/// vector of arguments, in the case of a table-valued function call,
/// whereas it's `None` in the case of a regular table name.
args: Option<Vec<FunctionArg>>,
/// A cols definition (i.e. `cols(view_schema name, view_name name, col_name name, col_type varchar, col_num int)`
/// when used with redshift pg_get_late_binding_view_cols/pg_get_cols)
columns_definition: Option<ColsDefinition>,
/// MSSQL-specific `WITH (...)` hints such as NOLOCK.
with_hints: Vec<Expr>,
},
Expand Down Expand Up @@ -543,6 +546,7 @@ impl fmt::Display for TableFactor {
name,
alias,
args,
columns_definition,
with_hints,
} => {
write!(f, "{}", name)?;
Expand All @@ -552,6 +556,9 @@ impl fmt::Display for TableFactor {
if let Some(alias) = alias {
write!(f, " AS {}", alias)?;
}
if let Some(columns_definition) = columns_definition {
write!(f, " {}", columns_definition)?;
}
if !with_hints.is_empty() {
write!(f, " WITH ({})", display_comma_separated(with_hints))?;
}
Expand Down
47 changes: 47 additions & 0 deletions src/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5356,6 +5356,7 @@ impl<'a> Parser<'a> {
} else {
None
};
let columns_definition = self.parse_redshift_columns_definition_list(&name)?;
let alias = self.parse_optional_table_alias(keywords::RESERVED_FOR_TABLE_ALIAS)?;
// MSSQL-specific table hints:
let mut with_hints = vec![];
Expand All @@ -5372,11 +5373,57 @@ impl<'a> Parser<'a> {
name,
alias,
args,
columns_definition,
with_hints,
})
}
}

fn parse_redshift_columns_definition_list(
&mut self,
name: &ObjectName,
) -> Result<Option<ColsDefinition>, ParserError> {
if !dialect_of!(self is RedshiftSqlDialect) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return Ok(None);
}

let fname = name
.0
.last()
.ok_or_else(|| {
ParserError::ParserError("Empty identifier vector for ObjectName".to_string())
})?
.value
.to_lowercase();

if fname == "pg_get_late_binding_view_cols"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

|| fname == "pg_get_cols"
|| fname == "pg_get_grantee_by_iam_role"
|| fname == "pg_get_iam_role_by_user"
{
if let Ok(col_definition_list_name) = self.parse_identifier() {
self.expect_token(&Token::LParen)?;
let names = self.parse_comma_separated(Parser::parse_ident_pair)?;
self.expect_token(&Token::RParen)?;
Ok(Some(ColsDefinition {
name: col_definition_list_name,
args: names,
}))
} else {
Ok(None)
}
} else {
Ok(None)
}
}

fn parse_ident_pair(&mut self) -> Result<IdentPair, ParserError> {
Ok(IdentPair(
self.parse_identifier()?,
self.parse_identifier()?,
))
}

pub fn parse_derived_table_factor(
&mut self,
lateral: IsLateral,
Expand Down
1 change: 1 addition & 0 deletions src/test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ pub fn table(name: impl Into<String>) -> TableFactor {
name: ObjectName(vec![Ident::new(name.into())]),
alias: None,
args: None,
columns_definition: None,
with_hints: vec![],
}
}
Expand Down
1 change: 1 addition & 0 deletions tests/sqlparser_bigquery.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ fn parse_table_identifiers() {
name: ObjectName(expected),
alias: None,
args: None,
columns_definition: None,
with_hints: vec![],
},
joins: vec![]
Expand Down
3 changes: 3 additions & 0 deletions tests/sqlparser_clickhouse.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ fn parse_map_access_expr() {
name: ObjectName(vec![Ident::new("foos")]),
alias: None,
args: None,
columns_definition: None,
with_hints: vec![],
},
joins: vec![]
Expand Down Expand Up @@ -164,11 +165,13 @@ fn parse_delimited_identifiers() {
name,
alias,
args,
columns_definition,
with_hints,
} => {
assert_eq!(vec![Ident::with_quote('"', "a table")], name.0);
assert_eq!(Ident::with_quote('"', "alias"), alias.unwrap().name);
assert!(args.is_none());
assert!(columns_definition.is_none());
assert!(with_hints.is_empty());
}
_ => panic!("Expecting TableFactor::Table"),
Expand Down
21 changes: 21 additions & 0 deletions tests/sqlparser_common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ fn parse_update_set_from() {
name: ObjectName(vec![Ident::new("t1")]),
alias: None,
args: None,
columns_definition: None,
with_hints: vec![],
},
joins: vec![],
Expand All @@ -236,6 +237,7 @@ fn parse_update_set_from() {
name: ObjectName(vec![Ident::new("t1")]),
alias: None,
args: None,
columns_definition: None,
with_hints: vec![],
},
joins: vec![],
Expand Down Expand Up @@ -298,6 +300,7 @@ fn parse_update_with_table_alias() {
columns: vec![],
}),
args: None,
columns_definition: None,
with_hints: vec![],
},
joins: vec![],
Expand Down Expand Up @@ -353,6 +356,7 @@ fn parse_delete_statement() {
name: ObjectName(vec![Ident::with_quote('"', "table")]),
alias: None,
args: None,
columns_definition: None,
with_hints: vec![],
},
table_name
Expand All @@ -379,6 +383,7 @@ fn parse_where_delete_statement() {
name: ObjectName(vec![Ident::new("foo")]),
alias: None,
args: None,
columns_definition: None,
with_hints: vec![],
},
table_name,
Expand Down Expand Up @@ -419,6 +424,7 @@ fn parse_where_delete_with_alias_statement() {
columns: vec![],
}),
args: None,
columns_definition: None,
with_hints: vec![],
},
table_name,
Expand All @@ -432,6 +438,7 @@ fn parse_where_delete_with_alias_statement() {
columns: vec![],
}),
args: None,
columns_definition: None,
with_hints: vec![],
}),
using
Expand Down Expand Up @@ -3391,6 +3398,7 @@ fn parse_interval_and_or_xor() {
}]),
alias: None,
args: None,
columns_definition: None,
with_hints: vec![],
},
joins: vec![],
Expand Down Expand Up @@ -3785,6 +3793,7 @@ fn parse_implicit_join() {
name: ObjectName(vec!["t1".into()]),
alias: None,
args: None,
columns_definition: None,
with_hints: vec![],
},
joins: vec![],
Expand All @@ -3794,6 +3803,7 @@ fn parse_implicit_join() {
name: ObjectName(vec!["t2".into()]),
alias: None,
args: None,
columns_definition: None,
with_hints: vec![],
},
joins: vec![],
Expand All @@ -3811,13 +3821,15 @@ fn parse_implicit_join() {
name: ObjectName(vec!["t1a".into()]),
alias: None,
args: None,
columns_definition: None,
with_hints: vec![],
},
joins: vec![Join {
relation: TableFactor::Table {
name: ObjectName(vec!["t1b".into()]),
alias: None,
args: None,
columns_definition: None,
with_hints: vec![],
},
join_operator: JoinOperator::Inner(JoinConstraint::Natural),
Expand All @@ -3828,13 +3840,15 @@ fn parse_implicit_join() {
name: ObjectName(vec!["t2a".into()]),
alias: None,
args: None,
columns_definition: None,
with_hints: vec![],
},
joins: vec![Join {
relation: TableFactor::Table {
name: ObjectName(vec!["t2b".into()]),
alias: None,
args: None,
columns_definition: None,
with_hints: vec![],
},
join_operator: JoinOperator::Inner(JoinConstraint::Natural),
Expand All @@ -3855,6 +3869,7 @@ fn parse_cross_join() {
name: ObjectName(vec![Ident::new("t2")]),
alias: None,
args: None,
columns_definition: None,
with_hints: vec![],
},
join_operator: JoinOperator::CrossJoin,
Expand All @@ -3875,6 +3890,7 @@ fn parse_joins_on() {
name: ObjectName(vec![Ident::new(relation.into())]),
alias,
args: None,
columns_definition: None,
with_hints: vec![],
},
join_operator: f(JoinConstraint::On(Expr::BinaryOp {
Expand Down Expand Up @@ -3944,6 +3960,7 @@ fn parse_joins_using() {
name: ObjectName(vec![Ident::new(relation.into())]),
alias,
args: None,
columns_definition: None,
with_hints: vec![],
},
join_operator: f(JoinConstraint::Using(vec!["c1".into()])),
Expand Down Expand Up @@ -4005,6 +4022,7 @@ fn parse_natural_join() {
name: ObjectName(vec![Ident::new("t2")]),
alias,
args: None,
columns_definition: None,
with_hints: vec![],
},
join_operator: f(JoinConstraint::Natural),
Expand Down Expand Up @@ -4272,6 +4290,7 @@ fn parse_derived_tables() {
name: ObjectName(vec!["t2".into()]),
alias: None,
args: None,
columns_definition: None,
with_hints: vec![],
},
join_operator: JoinOperator::Inner(JoinConstraint::Natural),
Expand Down Expand Up @@ -5564,6 +5583,7 @@ fn parse_merge() {
columns: vec![],
}),
args: None,
columns_definition: None,
with_hints: vec![],
}
);
Expand All @@ -5587,6 +5607,7 @@ fn parse_merge() {
name: ObjectName(vec![Ident::new("s"), Ident::new("foo")]),
alias: None,
args: None,
columns_definition: None,
with_hints: vec![],
},
joins: vec![],
Expand Down
2 changes: 2 additions & 0 deletions tests/sqlparser_hive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,11 +320,13 @@ fn parse_delimited_identifiers() {
name,
alias,
args,
columns_definition,
with_hints,
} => {
assert_eq!(vec![Ident::with_quote('"', "a table")], name.0);
assert_eq!(Ident::with_quote('"', "alias"), alias.unwrap().name);
assert!(args.is_none());
assert!(columns_definition.is_none());
assert!(with_hints.is_empty());
}
_ => panic!("Expecting TableFactor::Table"),
Expand Down
2 changes: 2 additions & 0 deletions tests/sqlparser_mssql.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,11 +152,13 @@ fn parse_delimited_identifiers() {
name,
alias,
args,
columns_definition,
with_hints,
} => {
assert_eq!(vec![Ident::with_quote('"', "a table")], name.0);
assert_eq!(Ident::with_quote('"', "alias"), alias.unwrap().name);
assert!(args.is_none());
assert!(columns_definition.is_none());
assert!(with_hints.is_empty());
}
_ => panic!("Expecting TableFactor::Table"),
Expand Down
3 changes: 3 additions & 0 deletions tests/sqlparser_mysql.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,6 +835,7 @@ fn parse_update_with_joins() {
columns: vec![]
}),
args: None,
columns_definition: None,
with_hints: vec![],
},
joins: vec![Join {
Expand All @@ -845,6 +846,7 @@ fn parse_update_with_joins() {
columns: vec![]
}),
args: None,
columns_definition: None,
with_hints: vec![],
},
join_operator: JoinOperator::Inner(JoinConstraint::On(Expr::BinaryOp {
Expand Down Expand Up @@ -966,6 +968,7 @@ fn parse_substring_in_select() {
}]),
alias: None,
args: None,
columns_definition: None,
with_hints: vec![]
},
joins: vec![]
Expand Down
2 changes: 2 additions & 0 deletions tests/sqlparser_postgres.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2188,11 +2188,13 @@ fn parse_delimited_identifiers() {
name,
alias,
args,
columns_definition,
with_hints,
} => {
assert_eq!(vec![Ident::with_quote('"', "a table")], name.0);
assert_eq!(Ident::with_quote('"', "alias"), alias.unwrap().name);
assert!(args.is_none());
assert!(columns_definition.is_none());
assert!(with_hints.is_empty());
}
_ => panic!("Expecting TableFactor::Table"),
Expand Down
Loading