Refactor parse_joins, pt.2: implicit/cross/natural joins

- reduce duplication in the handling of implicit/cross joins and make
  the flow of data slightly clearer by returning the `join` instead of
  pushing it and exiting early.

  (I wanted the block that currently returns `join` to return one of
  JoinOperator::* tags, so that `parse_table_factor` and the construction
  of the `Join` struct could happen after we've parsed the JOIN keywords,
  but that seems impossible.)

- move the check for the NATURAL keyword into the block that deals with 
  INNER/OUTER joins that support constraints (and thus can be preceded
  by "NATURAL")

- add a check for NATURAL not followed by a known join type with a test

- add more tests for NATURAL joins (we didn't have any), and fix
  whitespace bug in `to_string()` that was uncovered (we emitted an
  extra space: `foo NATURAL JOIN bar `)

Author: nickolay

src/sqlast/query.rs

@@ -271,34 +271,34 @@ impl ToString for Join {
         }
         fn suffix(constraint: &JoinConstraint) -> String {
             match constraint {
-                JoinConstraint::On(expr) => format!("ON {}", expr.to_string()),
-                JoinConstraint::Using(attrs) => format!("USING({})", attrs.join(", ")),
+                JoinConstraint::On(expr) => format!(" ON {}", expr.to_string()),
+                JoinConstraint::Using(attrs) => format!(" USING({})", attrs.join(", ")),
                 _ => "".to_string(),
             }
         }
         match &self.join_operator {
             JoinOperator::Inner(constraint) => format!(
-                " {}JOIN {} {}",
+                " {}JOIN {}{}",
                 prefix(constraint),
                 self.relation.to_string(),
                 suffix(constraint)
             ),
             JoinOperator::Cross => format!(" CROSS JOIN {}", self.relation.to_string()),
             JoinOperator::Implicit => format!(", {}", self.relation.to_string()),
             JoinOperator::LeftOuter(constraint) => format!(
-                " {}LEFT JOIN {} {}",
+                " {}LEFT JOIN {}{}",
                 prefix(constraint),
                 self.relation.to_string(),
                 suffix(constraint)
             ),
             JoinOperator::RightOuter(constraint) => format!(
-                " {}RIGHT JOIN {} {}",
+                " {}RIGHT JOIN {}{}",
                 prefix(constraint),
                 self.relation.to_string(),
                 suffix(constraint)
             ),
             JoinOperator::FullOuter(constraint) => format!(
-                " {}FULL JOIN {} {}",
+                " {}FULL JOIN {}{}",
                 prefix(constraint),
                 self.relation.to_string(),
                 suffix(constraint)

src/sqlparser.rs

@@ -1499,71 +1499,62 @@ impl Parser {
     fn parse_joins(&mut self) -> Result<Vec<Join>, ParserError> {
         let mut joins = vec![];
         loop {
-            let natural = match &self.peek_token() {
+            let join = match &self.peek_token() {
                 Some(Token::Comma) => {
                     self.next_token();
-                    let relation = self.parse_table_factor()?;
-                    let join = Join {
-                        relation,
+                    Join {
+                        relation: self.parse_table_factor()?,
                         join_operator: JoinOperator::Implicit,
-                    };
-                    joins.push(join);
-                    continue;
+                    }
                 }
                 Some(Token::SQLWord(kw)) if kw.keyword == "CROSS" => {
                     self.next_token();
                     self.expect_keyword("JOIN")?;
-                    let relation = self.parse_table_factor()?;
-                    let join = Join {
-                        relation,
+                    Join {
+                        relation: self.parse_table_factor()?,
                         join_operator: JoinOperator::Cross,
-                    };
-                    joins.push(join);
-                    continue;
-                }
-                Some(Token::SQLWord(kw)) if kw.keyword == "NATURAL" => {
-                    self.next_token();
-                    true
+                    }
                 }
-                Some(_) => false,
-                None => return Ok(joins),
-            };
-
-            let peek_keyword = if let Some(Token::SQLWord(kw)) = self.peek_token() {
-                kw.keyword
-            } else {
-                String::default()
-            };
+                _ => {
+                    let natural = self.parse_keyword("NATURAL");
+                    let peek_keyword = if let Some(Token::SQLWord(kw)) = self.peek_token() {
+                        kw.keyword
+                    } else {
+                        String::default()
+                    };
 
-            let join_operator_type = match peek_keyword.as_ref() {
-                "INNER" | "JOIN" => {
-                    let _ = self.parse_keyword("INNER");
-                    self.expect_keyword("JOIN")?;
-                    JoinOperator::Inner
-                }
-                kw @ "LEFT" | kw @ "RIGHT" | kw @ "FULL" => {
-                    let _ = self.next_token();
-                    let _ = self.parse_keyword("OUTER");
-                    self.expect_keyword("JOIN")?;
-                    match kw {
-                        "LEFT" => JoinOperator::LeftOuter,
-                        "RIGHT" => JoinOperator::RightOuter,
-                        "FULL" => JoinOperator::FullOuter,
-                        _ => unreachable!(),
+                    let join_operator_type = match peek_keyword.as_ref() {
+                        "INNER" | "JOIN" => {
+                            let _ = self.parse_keyword("INNER");
+                            self.expect_keyword("JOIN")?;
+                            JoinOperator::Inner
+                        }
+                        kw @ "LEFT" | kw @ "RIGHT" | kw @ "FULL" => {
+                            let _ = self.next_token();
+                            let _ = self.parse_keyword("OUTER");
+                            self.expect_keyword("JOIN")?;
+                            match kw {
+                                "LEFT" => JoinOperator::LeftOuter,
+                                "RIGHT" => JoinOperator::RightOuter,
+                                "FULL" => JoinOperator::FullOuter,
+                                _ => unreachable!(),
+                            }
+                        }
+                        _ if natural => {
+                            return self.expected("a join type after NATURAL", self.peek_token());
+                        }
+                        _ => break,
+                    };
+                    let relation = self.parse_table_factor()?;
+                    let join_constraint = self.parse_join_constraint(natural)?;
+                    Join {
+                        relation,
+                        join_operator: join_operator_type(join_constraint),
                     }
                 }
-                _ => break,
-            };
-            let relation = self.parse_table_factor()?;
-            let join_constraint = self.parse_join_constraint(natural)?;
-            let join = Join {
-                relation,
-                join_operator: join_operator_type(join_constraint),
             };
-
             joins.push(join);
         }
-
         Ok(joins)
     }
 

tests/sqlparser_common.rs

@@ -1218,6 +1218,43 @@ fn parse_joins_using() {
     );
 }
 
+#[test]
+fn parse_natural_join() {
+    fn natural_join(f: impl Fn(JoinConstraint) -> JoinOperator) -> Join {
+        Join {
+            relation: TableFactor::Table {
+                name: SQLObjectName(vec!["t2".to_string()]),
+                alias: None,
+                args: vec![],
+                with_hints: vec![],
+            },
+            join_operator: f(JoinConstraint::Natural),
+        }
+    }
+    assert_eq!(
+        verified_only_select("SELECT * FROM t1 NATURAL JOIN t2").joins,
+        vec![natural_join(JoinOperator::Inner)]
+    );
+    assert_eq!(
+        verified_only_select("SELECT * FROM t1 NATURAL LEFT JOIN t2").joins,
+        vec![natural_join(JoinOperator::LeftOuter)]
+    );
+    assert_eq!(
+        verified_only_select("SELECT * FROM t1 NATURAL RIGHT JOIN t2").joins,
+        vec![natural_join(JoinOperator::RightOuter)]
+    );
+    assert_eq!(
+        verified_only_select("SELECT * FROM t1 NATURAL FULL JOIN t2").joins,
+        vec![natural_join(JoinOperator::FullOuter)]
+    );
+
+    let sql = "SELECT * FROM t1 natural";
+    assert_eq!(
+        ParserError::ParserError("Expected a join type after NATURAL, found: EOF".to_string()),
+        parse_sql_statements(sql).unwrap_err(),
+    );
+}
+
 #[test]
 fn parse_complex_join() {
     let sql = "SELECT c1, c2 FROM t1, t4 JOIN t2 ON t2.c = t1.c LEFT JOIN t3 USING(q, c) WHERE t4.c = t1.c";