Merge pull request #116 from nickolay/pr/more-followups

nickolay · web-flow · commit 785754374963 · 2019-06-18T02:58:31.000+03:00
Support HAVING/LIMIT/OFFSET/FETCH without FROM and other follow-ups
diff --git 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, 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, LIMIT, OFFSET, FETCH,
+    ON, JOIN, INNER, CROSS, FULL, LEFT, RIGHT, NATURAL, USING,
 ];
 
 /// Can't be used as a column alias, so that `SELECT <expr> 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, HAVING, ORDER, LIMIT, OFFSET, FETCH, UNION, EXCEPT, INTERSECT,
     // Reserved only as a column alias in the `SELECT` clause:
     FROM,
 ];
diff --git 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<SQLIdent>,
 }
 
 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
@@ -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<u8, ParserError> {
         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),
@@ -1475,14 +1475,15 @@ impl Parser {
     fn parse_cte_list(&mut self) -> Result<Vec<Cte>, 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) {
@@ -1565,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 {
@@ -1605,6 +1611,10 @@ impl Parser {
 
     pub fn parse_table_and_joins(&mut self) -> Result<TableWithJoins, ParserError> {
         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") {
diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs
@@ -212,13 +212,13 @@ 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);
+
+    let ast = verified_query("SELECT 1 LIMIT 5");
+    assert_eq!(Some(ASTNode::SQLValue(Value::Long(5))), ast.limit);
 }
 
 #[test]
@@ -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(
@@ -1791,14 +1814,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 +1860,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
     );
 }
 
@@ -2201,6 +2220,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,15 +2234,15 @@ 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' 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,
@@ -2232,23 +2253,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",
     );
@@ -2273,63 +2280,28 @@ 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"),
         },
         _ => panic!("Test broke"),
     }
     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"),
         },

Original file line number	Diff line number	Diff line change
`@@ -163,18 +163,13 @@ impl ToString for SQLSelect {`
`163`	`163`	`/// number of columns in the query matches the number of columns in the query.`
`164`	`164`	`#[derive(Debug, Clone, PartialEq, Hash)]`
`165`	`165`	`pub struct Cte {`
`166`		`- pub alias: SQLIdent,`
	`166`	`+ pub alias: TableAlias,`
`167`	`167`	`pub query: SQLQuery,`
`168`		`- pub renamed_columns: Vec<SQLIdent>,`
`169`	`168`	`}`
`170`	`169`
`171`	`170`	`impl ToString for Cte {`
`172`	`171`	`fn to_string(&self) -> String {`
`173`		`- let mut s = self.alias.clone();`
`174`		`- if !self.renamed_columns.is_empty() {`
`175`		`- s += &format!(" ({})", comma_separated_string(&self.renamed_columns));`
`176`		`- }`
`177`		`- s + &format!(" AS ({})", self.query.to_string())`
	`172`	`+ format!("{} AS ({})", self.alias.to_string(), self.query.to_string())`
`178`	`173`	`}`
`179`	`174`	`}`
`180`	`175`