From 4f239745bc7a8978e35c0deffe9ec79acb6a2ba7 Mon Sep 17 00:00:00 2001 From: Nickolay Ponomarev Date: Sun, 16 Jun 2019 21:01:29 +0300 Subject: [PATCH 1/7] Update comments now that get_precedence is no more --- src/sqlparser.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sqlparser.rs b/src/sqlparser.rs index 6e9941801..0b982f808 100644 --- a/src/sqlparser.rs +++ b/src/sqlparser.rs @@ -574,13 +574,13 @@ impl Parser { self.expected("IN or BETWEEN after NOT", self.peek_token()) } } - // Can only happen if `get_precedence` got out of sync with this function + // Can only happen if `get_next_precedence` got out of sync with this function _ => panic!("No infix parser for token {:?}", tok), } } else if Token::DoubleColon == tok { self.parse_pg_cast(expr) } else { - // Can only happen if `get_precedence` got out of sync with this function + // Can only happen if `get_next_precedence` got out of sync with this function panic!("No infix parser for token {:?}", tok) } } @@ -636,7 +636,7 @@ impl Parser { /// Get the precedence of the next token pub fn get_next_precedence(&self) -> Result { if let Some(token) = self.peek_token() { - debug!("get_precedence() {:?}", token); + debug!("get_next_precedence() {:?}", token); match &token { Token::SQLWord(k) if k.keyword == "OR" => Ok(5), From 3c073a4c34abfdc4f1ba59dc50afc232db5ea340 Mon Sep 17 00:00:00 2001 From: Nickolay Ponomarev Date: Sun, 16 Jun 2019 21:11:51 +0300 Subject: [PATCH 2/7] Use TableAlias in Cte --- src/sqlast/query.rs | 9 ++------- src/sqlparser.rs | 7 ++++--- tests/sqlparser_common.rs | 12 ++++-------- 3 files changed, 10 insertions(+), 18 deletions(-) diff --git a/src/sqlast/query.rs b/src/sqlast/query.rs index f9e9eaf6e..4c23c0af1 100644 --- a/src/sqlast/query.rs +++ b/src/sqlast/query.rs @@ -163,18 +163,13 @@ impl ToString for SQLSelect { /// number of columns in the query matches the number of columns in the query. #[derive(Debug, Clone, PartialEq, Hash)] pub struct Cte { - pub alias: SQLIdent, + pub alias: TableAlias, pub query: SQLQuery, - pub renamed_columns: Vec, } impl ToString for Cte { fn to_string(&self) -> String { - let mut s = self.alias.clone(); - if !self.renamed_columns.is_empty() { - s += &format!(" ({})", comma_separated_string(&self.renamed_columns)); - } - s + &format!(" AS ({})", self.query.to_string()) + format!("{} AS ({})", self.alias.to_string(), self.query.to_string()) } } diff --git a/src/sqlparser.rs b/src/sqlparser.rs index 0b982f808..b55aede08 100644 --- a/src/sqlparser.rs +++ b/src/sqlparser.rs @@ -1475,14 +1475,15 @@ impl Parser { fn parse_cte_list(&mut self) -> Result, ParserError> { let mut cte = vec![]; loop { - let alias = self.parse_identifier()?; - let renamed_columns = self.parse_parenthesized_column_list(Optional)?; + let alias = TableAlias { + name: self.parse_identifier()?, + columns: self.parse_parenthesized_column_list(Optional)?, + }; self.expect_keyword("AS")?; self.expect_token(&Token::LParen)?; cte.push(Cte { alias, query: self.parse_query()?, - renamed_columns, }); self.expect_token(&Token::RParen)?; if !self.consume_token(&Token::Comma) { diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index a32ac6eac..c8b6ffada 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -1791,14 +1791,10 @@ fn parse_ctes() { fn assert_ctes_in_select(expected: &[&str], sel: &SQLQuery) { let mut i = 0; for exp in expected { - let Cte { - query, - alias, - renamed_columns, - } = &sel.ctes[i]; + let Cte { alias, query } = &sel.ctes[i]; assert_eq!(*exp, query.to_string()); - assert_eq!(if i == 0 { "a" } else { "b" }, alias); - assert!(renamed_columns.is_empty()); + assert_eq!(if i == 0 { "a" } else { "b" }, alias.name); + assert!(alias.columns.is_empty()); i += 1; } } @@ -1841,7 +1837,7 @@ fn parse_cte_renamed_columns() { let query = all_dialects().verified_query(sql); assert_eq!( vec!["col1", "col2"], - query.ctes.first().unwrap().renamed_columns + query.ctes.first().unwrap().alias.columns ); } From f87e8d51584c8cff40ac4f2dc75309d49d491921 Mon Sep 17 00:00:00 2001 From: Nickolay Ponomarev Date: Mon, 17 Jun 2019 00:36:57 +0300 Subject: [PATCH 3/7] Don't duplicate all the parse_simple_select assertions in the LIMIT test --- tests/sqlparser_common.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index c8b6ffada..95f39911a 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -212,13 +212,10 @@ fn parse_simple_select() { } #[test] -fn parse_select_with_limit_but_no_where() { - let sql = "SELECT id, fname, lname FROM customer LIMIT 5"; - let select = verified_only_select(sql); - assert_eq!(false, select.distinct); - assert_eq!(3, select.projection.len()); - let select = verified_query(sql); - assert_eq!(Some(ASTNode::SQLValue(Value::Long(5))), select.limit); +fn parse_limit_is_not_an_alias() { + // In dialects supporting LIMIT it shouldn't be parsed as a table alias + let ast = verified_query("SELECT id FROM customer LIMIT 1"); + assert_eq!(Some(ASTNode::SQLValue(Value::Long(1))), ast.limit); } #[test] From c1509b36ec1f2df5bd257453587fa74ba008500f Mon Sep 17 00:00:00 2001 From: Nickolay Ponomarev Date: Mon, 17 Jun 2019 00:49:25 +0300 Subject: [PATCH 4/7] Use FETCH_FIRST_TWO_ROWS_ONLY in tests to reduce duplication --- tests/sqlparser_common.rs | 77 +++++++-------------------------------- 1 file changed, 13 insertions(+), 64 deletions(-) diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 95f39911a..3fae12184 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -2206,15 +2206,13 @@ fn parse_singular_row_offset() { #[test] fn parse_fetch() { + const FETCH_FIRST_TWO_ROWS_ONLY: Fetch = Fetch { + with_ties: false, + percent: false, + quantity: Some(ASTNode::SQLValue(Value::Long(2))), + }; let ast = verified_query("SELECT foo FROM bar FETCH FIRST 2 ROWS ONLY"); - assert_eq!( - ast.fetch, - Some(Fetch { - with_ties: false, - percent: false, - quantity: Some(ASTNode::SQLValue(Value::Long(2))), - }) - ); + assert_eq!(ast.fetch, Some(FETCH_FIRST_TWO_ROWS_ONLY)); let ast = verified_query("SELECT foo FROM bar FETCH FIRST ROWS ONLY"); assert_eq!( ast.fetch, @@ -2225,23 +2223,9 @@ fn parse_fetch() { }) ); let ast = verified_query("SELECT foo FROM bar WHERE foo = 4 FETCH FIRST 2 ROWS ONLY"); - assert_eq!( - ast.fetch, - Some(Fetch { - with_ties: false, - percent: false, - quantity: Some(ASTNode::SQLValue(Value::Long(2))), - }) - ); + assert_eq!(ast.fetch, Some(FETCH_FIRST_TWO_ROWS_ONLY)); let ast = verified_query("SELECT foo FROM bar ORDER BY baz FETCH FIRST 2 ROWS ONLY"); - assert_eq!( - ast.fetch, - Some(Fetch { - with_ties: false, - percent: false, - quantity: Some(ASTNode::SQLValue(Value::Long(2))), - }) - ); + assert_eq!(ast.fetch, Some(FETCH_FIRST_TWO_ROWS_ONLY)); let ast = verified_query( "SELECT foo FROM bar WHERE foo = 4 ORDER BY baz FETCH FIRST 2 ROWS WITH TIES", ); @@ -2266,36 +2250,15 @@ fn parse_fetch() { "SELECT foo FROM bar WHERE foo = 4 ORDER BY baz OFFSET 2 ROWS FETCH FIRST 2 ROWS ONLY", ); assert_eq!(ast.offset, Some(ASTNode::SQLValue(Value::Long(2)))); - assert_eq!( - ast.fetch, - Some(Fetch { - with_ties: false, - percent: false, - quantity: Some(ASTNode::SQLValue(Value::Long(2))), - }) - ); + assert_eq!(ast.fetch, Some(FETCH_FIRST_TWO_ROWS_ONLY)); let ast = verified_query( "SELECT foo FROM (SELECT * FROM bar FETCH FIRST 2 ROWS ONLY) FETCH FIRST 2 ROWS ONLY", ); - assert_eq!( - ast.fetch, - Some(Fetch { - with_ties: false, - percent: false, - quantity: Some(ASTNode::SQLValue(Value::Long(2))), - }) - ); + assert_eq!(ast.fetch, Some(FETCH_FIRST_TWO_ROWS_ONLY)); match ast.body { SQLSetExpr::Select(s) => match only(s.from).relation { TableFactor::Derived { subquery, .. } => { - assert_eq!( - subquery.fetch, - Some(Fetch { - with_ties: false, - percent: false, - quantity: Some(ASTNode::SQLValue(Value::Long(2))), - }) - ); + assert_eq!(subquery.fetch, Some(FETCH_FIRST_TWO_ROWS_ONLY)); } _ => panic!("Test broke"), }, @@ -2303,26 +2266,12 @@ fn parse_fetch() { } let ast = verified_query("SELECT foo FROM (SELECT * FROM bar OFFSET 2 ROWS FETCH FIRST 2 ROWS ONLY) OFFSET 2 ROWS FETCH FIRST 2 ROWS ONLY"); assert_eq!(ast.offset, Some(ASTNode::SQLValue(Value::Long(2)))); - assert_eq!( - ast.fetch, - Some(Fetch { - with_ties: false, - percent: false, - quantity: Some(ASTNode::SQLValue(Value::Long(2))), - }) - ); + assert_eq!(ast.fetch, Some(FETCH_FIRST_TWO_ROWS_ONLY)); match ast.body { SQLSetExpr::Select(s) => match only(s.from).relation { TableFactor::Derived { subquery, .. } => { assert_eq!(subquery.offset, Some(ASTNode::SQLValue(Value::Long(2)))); - assert_eq!( - subquery.fetch, - Some(Fetch { - with_ties: false, - percent: false, - quantity: Some(ASTNode::SQLValue(Value::Long(2))), - }) - ); + assert_eq!(subquery.fetch, Some(FETCH_FIRST_TWO_ROWS_ONLY)); } _ => panic!("Test broke"), }, From d60bdc0b92900b96b9297264fccc3b9519dd3217 Mon Sep 17 00:00:00 2001 From: Nickolay Ponomarev Date: Mon, 17 Jun 2019 00:54:37 +0300 Subject: [PATCH 5/7] Allow LIMIT/OFFSET/FETCH without FROM Postgres allows it, as does ANSI SQL per the definition: https://jakewheat.github.io/sql-overview/sql-2011-foundation-grammar.html#_7_13_query_expression --- src/dialect/keywords.rs | 6 +++--- tests/sqlparser_common.rs | 7 +++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/dialect/keywords.rs b/src/dialect/keywords.rs index a6188c993..dd1548d9b 100644 --- a/src/dialect/keywords.rs +++ b/src/dialect/keywords.rs @@ -420,16 +420,16 @@ define_keywords!( /// can be parsed unambiguously without looking ahead. pub const RESERVED_FOR_TABLE_ALIAS: &[&str] = &[ // Reserved as both a table and a column alias: - WITH, SELECT, WHERE, GROUP, ORDER, UNION, EXCEPT, INTERSECT, + WITH, SELECT, WHERE, GROUP, ORDER, LIMIT, OFFSET, FETCH, UNION, EXCEPT, INTERSECT, // Reserved only as a table alias in the `FROM`/`JOIN` clauses: - ON, JOIN, INNER, CROSS, FULL, LEFT, RIGHT, NATURAL, USING, LIMIT, OFFSET, FETCH, + ON, JOIN, INNER, CROSS, FULL, LEFT, RIGHT, NATURAL, USING, ]; /// Can't be used as a column alias, so that `SELECT alias` /// can be parsed unambiguously without looking ahead. pub const RESERVED_FOR_COLUMN_ALIAS: &[&str] = &[ // Reserved as both a table and a column alias: - WITH, SELECT, WHERE, GROUP, ORDER, UNION, EXCEPT, INTERSECT, + WITH, SELECT, WHERE, GROUP, ORDER, LIMIT, OFFSET, FETCH, UNION, EXCEPT, INTERSECT, // Reserved only as a column alias in the `SELECT` clause: FROM, ]; diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 3fae12184..4984c4838 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -216,6 +216,9 @@ fn parse_limit_is_not_an_alias() { // In dialects supporting LIMIT it shouldn't be parsed as a table alias let ast = verified_query("SELECT id FROM customer LIMIT 1"); assert_eq!(Some(ASTNode::SQLValue(Value::Long(1))), ast.limit); + + let ast = verified_query("SELECT 1 LIMIT 5"); + assert_eq!(Some(ASTNode::SQLValue(Value::Long(5))), ast.limit); } #[test] @@ -2194,6 +2197,8 @@ fn parse_offset() { }, _ => panic!("Test broke"), } + let ast = verified_query("SELECT 'foo' OFFSET 0 ROWS"); + assert_eq!(ast.offset, Some(ASTNode::SQLValue(Value::Long(0)))); } #[test] @@ -2213,6 +2218,8 @@ fn parse_fetch() { }; let ast = verified_query("SELECT foo FROM bar FETCH FIRST 2 ROWS ONLY"); assert_eq!(ast.fetch, Some(FETCH_FIRST_TWO_ROWS_ONLY)); + let ast = verified_query("SELECT 'foo' FETCH FIRST 2 ROWS ONLY"); + assert_eq!(ast.fetch, Some(FETCH_FIRST_TWO_ROWS_ONLY)); let ast = verified_query("SELECT foo FROM bar FETCH FIRST ROWS ONLY"); assert_eq!( ast.fetch, From eb3450dd51f07805f292a42bc22cb4fda94ee9d5 Mon Sep 17 00:00:00 2001 From: Nickolay Ponomarev Date: Mon, 17 Jun 2019 01:04:04 +0300 Subject: [PATCH 6/7] Support HAVING without GROUP BY ...which is weird but allowed: https://jakewheat.github.io/sql-overview/sql-2011-foundation-grammar.html#table-expression https://dba.stackexchange.com/a/57453/15599 Also add a test for GROUP BY .. HAVING --- src/dialect/keywords.rs | 4 ++-- src/sqlparser.rs | 5 +++++ tests/sqlparser_common.rs | 23 +++++++++++++++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/dialect/keywords.rs b/src/dialect/keywords.rs index dd1548d9b..c3489ac67 100644 --- a/src/dialect/keywords.rs +++ b/src/dialect/keywords.rs @@ -420,7 +420,7 @@ define_keywords!( /// can be parsed unambiguously without looking ahead. pub const RESERVED_FOR_TABLE_ALIAS: &[&str] = &[ // Reserved as both a table and a column alias: - WITH, SELECT, WHERE, GROUP, ORDER, LIMIT, OFFSET, FETCH, UNION, EXCEPT, INTERSECT, + WITH, SELECT, WHERE, GROUP, HAVING, ORDER, LIMIT, OFFSET, FETCH, UNION, EXCEPT, INTERSECT, // Reserved only as a table alias in the `FROM`/`JOIN` clauses: ON, JOIN, INNER, CROSS, FULL, LEFT, RIGHT, NATURAL, USING, ]; @@ -429,7 +429,7 @@ pub const RESERVED_FOR_TABLE_ALIAS: &[&str] = &[ /// can be parsed unambiguously without looking ahead. pub const RESERVED_FOR_COLUMN_ALIAS: &[&str] = &[ // Reserved as both a table and a column alias: - WITH, SELECT, WHERE, GROUP, ORDER, LIMIT, OFFSET, FETCH, UNION, EXCEPT, INTERSECT, + WITH, SELECT, WHERE, GROUP, HAVING, ORDER, LIMIT, OFFSET, FETCH, UNION, EXCEPT, INTERSECT, // Reserved only as a column alias in the `SELECT` clause: FROM, ]; diff --git a/src/sqlparser.rs b/src/sqlparser.rs index b55aede08..3e11f2e9c 100644 --- a/src/sqlparser.rs +++ b/src/sqlparser.rs @@ -1566,6 +1566,11 @@ impl Parser { } let projection = self.parse_select_list()?; + // Note that for keywords to be properly handled here, they need to be + // added to `RESERVED_FOR_COLUMN_ALIAS` / `RESERVED_FOR_TABLE_ALIAS`, + // otherwise they may be parsed as an alias as part of the `projection` + // or `from`. + let mut from = vec![]; if self.parse_keyword("FROM") { loop { diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 4984c4838..7b4341fb8 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -789,6 +789,29 @@ fn parse_select_group_by() { ); } +#[test] +fn parse_select_having() { + let sql = "SELECT foo FROM bar GROUP BY foo HAVING COUNT(*) > 1"; + let select = verified_only_select(sql); + assert_eq!( + Some(ASTNode::SQLBinaryOp { + left: Box::new(ASTNode::SQLFunction(SQLFunction { + name: SQLObjectName(vec!["COUNT".to_string()]), + args: vec![ASTNode::SQLWildcard], + over: None, + distinct: false + })), + op: SQLBinaryOperator::Gt, + right: Box::new(ASTNode::SQLValue(Value::Long(1))) + }), + select.having + ); + + let sql = "SELECT 'foo' HAVING 1 = 1"; + let select = verified_only_select(sql); + assert!(select.having.is_some()); +} + #[test] fn parse_limit_accepts_all() { one_statement_parses_to( From a37ba089ecc13bb6539e18e70dafd67173c487dc Mon Sep 17 00:00:00 2001 From: Nickolay Ponomarev Date: Mon, 17 Jun 2019 10:55:12 +0300 Subject: [PATCH 7/7] Add a comment about RESERVED_FOR_TABLE_ALIAS to parse_table_and_joins --- src/sqlparser.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/sqlparser.rs b/src/sqlparser.rs index 3e11f2e9c..6924a9329 100644 --- a/src/sqlparser.rs +++ b/src/sqlparser.rs @@ -1611,6 +1611,10 @@ impl Parser { pub fn parse_table_and_joins(&mut self) -> Result { let relation = self.parse_table_factor()?; + + // Note that for keywords to be properly handled here, they need to be + // added to `RESERVED_FOR_TABLE_ALIAS`, otherwise they may be parsed as + // a table alias. let mut joins = vec![]; loop { let join = if self.parse_keyword("CROSS") {