From 23f5c7e7ce3146b54411042c0b7b85c1ee721561 Mon Sep 17 00:00:00 2001 From: Nickolay Ponomarev Date: Fri, 31 Jul 2020 16:49:58 +0300 Subject: [PATCH 1/2] Move parse_column_def below parse_columns ...because in the section related to CREATE TABLE parsing, the callers are defined above the callees. --- src/parser.rs | 55 +++++++++++++++++++++++++-------------------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/src/parser.rs b/src/parser.rs index 5f113a07..c38a8f81 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -1191,6 +1191,33 @@ impl<'a> Parser<'a> { }) } + fn parse_columns(&mut self) -> Result<(Vec, Vec), ParserError> { + let mut columns = vec![]; + let mut constraints = vec![]; + if !self.consume_token(&Token::LParen) || self.consume_token(&Token::RParen) { + return Ok((columns, constraints)); + } + + loop { + if let Some(constraint) = self.parse_optional_table_constraint()? { + constraints.push(constraint); + } else if let Token::Word(_) = self.peek_token() { + columns.push(self.parse_column_def()?); + } else { + return self.expected("column name or constraint definition", self.peek_token()); + } + let comma = self.consume_token(&Token::Comma); + if self.consume_token(&Token::RParen) { + // allow a trailing comma, even though it's not in standard + break; + } else if !comma { + return self.expected("',' or ')' after column definition", self.peek_token()); + } + } + + Ok((columns, constraints)) + } + fn parse_column_def(&mut self) -> Result { let name = self.parse_identifier()?; let data_type = self.parse_data_type()?; @@ -1214,34 +1241,6 @@ impl<'a> Parser<'a> { }) } - fn parse_columns(&mut self) -> Result<(Vec, Vec), ParserError> { - let mut columns = vec![]; - let mut constraints = vec![]; - if !self.consume_token(&Token::LParen) || self.consume_token(&Token::RParen) { - return Ok((columns, constraints)); - } - - loop { - if let Some(constraint) = self.parse_optional_table_constraint()? { - constraints.push(constraint); - } else if let Token::Word(_) = self.peek_token() { - let column_def = self.parse_column_def()?; - columns.push(column_def); - } else { - return self.expected("column name or constraint definition", self.peek_token()); - } - let comma = self.consume_token(&Token::Comma); - if self.consume_token(&Token::RParen) { - // allow a trailing comma, even though it's not in standard - break; - } else if !comma { - return self.expected("',' or ')' after column definition", self.peek_token()); - } - } - - Ok((columns, constraints)) - } - pub fn parse_column_option_def(&mut self) -> Result { let name = if self.parse_keyword(Keyword::CONSTRAINT) { Some(self.parse_identifier()?) From 66505ebf9e21d62061eb9df6e37441b5eae007c3 Mon Sep 17 00:00:00 2001 From: Nickolay Ponomarev Date: Mon, 10 Aug 2020 17:12:33 +0300 Subject: [PATCH 2/2] Don't fail parsing a column definition with unexpected tokens Since PR https://github.com/ballista-compute/sqlparser-rs/pull/93 `parse_column_def` parses a set of column options in a loop, e.g. given: ``` _______ column_def _______ CREATE TABLE foo (bar INT NOT NULL DEFAULT 1, ) -------- --------- option 1 option 2 ```` it parses column options until it encounters one of the delimiter tokens First when we only supported `CREATE TABLE`, the set of delimiters that stopped the parsing used to be `Token::Comma | Token::RParen`. Then we added support for `ALTER TABLE ADD COLUMN `. Turns out the parser started to bail if the statement ended with a semicolon, while attempting to parse the semicolon as a column option, as we forgot to add it to the set of delimiter tokens. This was recently fixed in https://github.com/ballista-compute/sqlparser-rs/pull/246 by including Token::SemiColon to the list, but it felt wrong to have to update this list, and to have a common list of delimiters for two different contexts (CREATE TABLE with parens vs ALTER TABLE ADD COLUMN without parens). Also our current approach cannot handle multiple statements NOT separated by a semicolon, as is common in MS SQL DDL. We don't explicitly support it in `parse_statements`, but that's a use-case like to keep in mind nevertheless. --- src/parser.rs | 59 ++++++++++++++++++++++----------------- tests/sqlparser_common.rs | 8 +++++- 2 files changed, 40 insertions(+), 27 deletions(-) diff --git a/src/parser.rs b/src/parser.rs index c38a8f81..431984a1 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -1228,10 +1228,21 @@ impl<'a> Parser<'a> { }; let mut options = vec![]; loop { - match self.peek_token() { - Token::EOF | Token::Comma | Token::RParen | Token::SemiColon => break, - _ => options.push(self.parse_column_option_def()?), - } + if self.parse_keyword(Keyword::CONSTRAINT) { + let name = Some(self.parse_identifier()?); + if let Some(option) = self.parse_optional_column_option()? { + options.push(ColumnOptionDef { name, option }); + } else { + return self.expected( + "constraint details after CONSTRAINT ", + self.peek_token(), + ); + } + } else if let Some(option) = self.parse_optional_column_option()? { + options.push(ColumnOptionDef { name: None, option }); + } else { + break; + }; } Ok(ColumnDef { name, @@ -1241,23 +1252,17 @@ impl<'a> Parser<'a> { }) } - pub fn parse_column_option_def(&mut self) -> Result { - let name = if self.parse_keyword(Keyword::CONSTRAINT) { - Some(self.parse_identifier()?) - } else { - None - }; - - let option = if self.parse_keywords(&[Keyword::NOT, Keyword::NULL]) { - ColumnOption::NotNull + pub fn parse_optional_column_option(&mut self) -> Result, ParserError> { + if self.parse_keywords(&[Keyword::NOT, Keyword::NULL]) { + Ok(Some(ColumnOption::NotNull)) } else if self.parse_keyword(Keyword::NULL) { - ColumnOption::Null + Ok(Some(ColumnOption::Null)) } else if self.parse_keyword(Keyword::DEFAULT) { - ColumnOption::Default(self.parse_expr()?) + Ok(Some(ColumnOption::Default(self.parse_expr()?))) } else if self.parse_keywords(&[Keyword::PRIMARY, Keyword::KEY]) { - ColumnOption::Unique { is_primary: true } + Ok(Some(ColumnOption::Unique { is_primary: true })) } else if self.parse_keyword(Keyword::UNIQUE) { - ColumnOption::Unique { is_primary: false } + Ok(Some(ColumnOption::Unique { is_primary: false })) } else if self.parse_keyword(Keyword::REFERENCES) { let foreign_table = self.parse_object_name()?; // PostgreSQL allows omitting the column list and @@ -1276,32 +1281,34 @@ impl<'a> Parser<'a> { break; } } - ColumnOption::ForeignKey { + Ok(Some(ColumnOption::ForeignKey { foreign_table, referred_columns, on_delete, on_update, - } + })) } else if self.parse_keyword(Keyword::CHECK) { self.expect_token(&Token::LParen)?; let expr = self.parse_expr()?; self.expect_token(&Token::RParen)?; - ColumnOption::Check(expr) + Ok(Some(ColumnOption::Check(expr))) } else if self.parse_keyword(Keyword::AUTO_INCREMENT) && dialect_of!(self is MySqlDialect | GenericDialect) { // Support AUTO_INCREMENT for MySQL - ColumnOption::DialectSpecific(vec![Token::make_keyword("AUTO_INCREMENT")]) + Ok(Some(ColumnOption::DialectSpecific(vec![ + Token::make_keyword("AUTO_INCREMENT"), + ]))) } else if self.parse_keyword(Keyword::AUTOINCREMENT) && dialect_of!(self is SQLiteDialect | GenericDialect) { // Support AUTOINCREMENT for SQLite - ColumnOption::DialectSpecific(vec![Token::make_keyword("AUTOINCREMENT")]) + Ok(Some(ColumnOption::DialectSpecific(vec![ + Token::make_keyword("AUTOINCREMENT"), + ]))) } else { - return self.expected("column option", self.peek_token()); - }; - - Ok(ColumnOptionDef { name, option }) + Ok(None) + } } pub fn parse_referential_action(&mut self) -> Result { diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index f3234e99..c06e4487 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -1142,7 +1142,13 @@ fn parse_create_table() { assert!(res .unwrap_err() .to_string() - .contains("Expected column option, found: GARBAGE")); + .contains("Expected \',\' or \')\' after column definition, found: GARBAGE")); + + let res = parse_sql_statements("CREATE TABLE t (a int NOT NULL CONSTRAINT foo)"); + assert!(res + .unwrap_err() + .to_string() + .contains("Expected constraint details after CONSTRAINT ")); } #[test]