Merge pull request #131 from nickolay/pr/refactoring

Assorted code simplification and doc improvements

src/ast/mod.rs

@@ -71,6 +71,16 @@ where
 /// Identifier name, in the originally quoted form (e.g. `"id"`)
 pub type Ident = String;
 
+/// A name of a table, view, custom type, etc., possibly multi-part, i.e. db.schema.obj
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub struct ObjectName(pub Vec<Ident>);
+
+impl fmt::Display for ObjectName {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{}", display_separated(&self.0, "."))
+    }
+}
+
 /// An SQL expression of any type.
 ///
 /// The parser does not distinguish between expressions of different types
@@ -165,12 +175,9 @@ pub enum Expr {
 impl fmt::Display for Expr {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
-            Expr::Identifier(s) => write!(f, "{}", s),
+            Expr::Identifier(s) => f.write_str(s),
             Expr::Wildcard => f.write_str("*"),
-            Expr::QualifiedWildcard(q) => {
+            Expr::QualifiedWildcard(q) => write!(f, "{}.*", display_separated(q, ".")),
-                write!(f, "{}", display_separated(q, "."))?;
-                f.write_str(".*")
-            }
             Expr::CompoundIdentifier(s) => write!(f, "{}", display_separated(s, ".")),
             Expr::IsNull(ast) => write!(f, "{} IS NULL", ast),
             Expr::IsNotNull(ast) => write!(f, "{} IS NOT NULL", ast),
@@ -285,11 +292,16 @@ impl fmt::Display for WindowSpec {
 
 /// Specifies the data processed by a window function, e.g.
 /// `RANGE UNBOUNDED PRECEDING` or `ROWS BETWEEN 5 PRECEDING AND CURRENT ROW`.
+///
+/// Note: The parser does not validate the specified bounds; the caller should
+/// reject invalid bounds like `ROWS UNBOUNDED FOLLOWING` before execution.
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct WindowFrame {
     pub units: WindowFrameUnits,
     pub start_bound: WindowFrameBound,
-    /// The right bound of the `BETWEEN .. AND` clause.
+    /// The right bound of the `BETWEEN .. AND` clause. The end bound of `None`
+    /// indicates the shorthand form (e.g. `ROWS 1 PRECEDING`), which must
+    /// behave the same as `end_bound = WindowFrameBound::CurrentRow`.
     pub end_bound: Option<WindowFrameBound>,
     // TBD: EXCLUDE
 }
@@ -327,14 +339,14 @@ impl FromStr for WindowFrameUnits {
     }
 }
 
+/// Specifies [WindowFrame]'s `start_bound` and `end_bound`
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub enum WindowFrameBound {
     /// `CURRENT ROW`
     CurrentRow,
     /// `<N> PRECEDING` or `UNBOUNDED PRECEDING`
     Preceding(Option<u64>),
-    /// `<N> FOLLOWING` or `UNBOUNDED FOLLOWING`. This can only appear in
+    /// `<N> FOLLOWING` or `UNBOUNDED FOLLOWING`.
-    /// [WindowFrame::end_bound].
     Following(Option<u64>),
 }
 
@@ -416,11 +428,16 @@ pub enum Statement {
         name: ObjectName,
         operation: AlterTableOperation,
     },
-    /// DROP TABLE
+    /// DROP
     Drop {
+        /// The type of the object to drop: TABLE, VIEW, etc.
         object_type: ObjectType,
+        /// An optional `IF EXISTS` clause. (Non-standard.)
         if_exists: bool,
+        /// One or more objects to drop. (ANSI SQL requires exactly one.)
         names: Vec<ObjectName>,
+        /// Whether `CASCADE` was specified. This will be `false` when
+        /// `RESTRICT` or no drop behavior at all was specified.
         cascade: bool,
     },
     /// `{ BEGIN [ TRANSACTION | WORK ] | START TRANSACTION } ...`
@@ -596,16 +613,6 @@ impl fmt::Display for Statement {
     }
 }
 
-/// A name of a table, view, custom type, etc., possibly multi-part, i.e. db.schema.obj
-#[derive(Debug, Clone, PartialEq, Eq, Hash)]
-pub struct ObjectName(pub Vec<Ident>);
-
-impl fmt::Display for ObjectName {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(f, "{}", display_separated(&self.0, "."))
-    }
-}
-
 /// SQL assignment `foo = expr` as used in SQLUpdate
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct Assignment {
@@ -660,19 +667,15 @@ pub enum FileFormat {
 impl fmt::Display for FileFormat {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         use self::FileFormat::*;
-        write!(
+        f.write_str(match self {
-            f,
+            TEXTFILE => "TEXTFILE",
-            "{}",
+            SEQUENCEFILE => "SEQUENCEFILE",
-            match self {
+            ORC => "ORC",
-                TEXTFILE => "TEXTFILE",
+            PARQUET => "PARQUET",
-                SEQUENCEFILE => "SEQUENCEFILE",
+            AVRO => "AVRO",
-                ORC => "ORC",
+            RCFILE => "RCFILE",
-                PARQUET => "PARQUET",
+            JSONFILE => "TEXTFILE",
-                AVRO => "AVRO",
+        })
-                RCFILE => "RCFILE",
-                JSONFILE => "TEXTFILE",
-            }
-        )
     }
 }
 
@@ -707,14 +710,10 @@ pub enum ObjectType {
 
 impl fmt::Display for ObjectType {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(
+        f.write_str(match self {
-            f,
+            ObjectType::Table => "TABLE",
-            "{}",
+            ObjectType::View => "VIEW",
-            match self {
+        })
-                ObjectType::Table => "TABLE",
-                ObjectType::View => "VIEW",
-            }
-        )
     }
 }
 
@@ -740,7 +739,7 @@ impl fmt::Display for TransactionMode {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         use TransactionMode::*;
         match self {
-            AccessMode(access_mode) => write!(f, "{}", access_mode.to_string()),
+            AccessMode(access_mode) => write!(f, "{}", access_mode),
             IsolationLevel(iso_level) => write!(f, "ISOLATION LEVEL {}", iso_level),
         }
     }
@@ -755,14 +754,10 @@ pub enum TransactionAccessMode {
 impl fmt::Display for TransactionAccessMode {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         use TransactionAccessMode::*;
-        write!(
+        f.write_str(match self {
-            f,
+            ReadOnly => "READ ONLY",
-            "{}",
+            ReadWrite => "READ WRITE",
-            match self {
+        })
-                ReadOnly => "READ ONLY",
-                ReadWrite => "READ WRITE",
-            }
-        )
     }
 }
 
@@ -777,15 +772,11 @@ pub enum TransactionIsolationLevel {
 impl fmt::Display for TransactionIsolationLevel {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         use TransactionIsolationLevel::*;
-        write!(
+        f.write_str(match self {
-            f,
+            ReadUncommitted => "READ UNCOMMITTED",
-            "{}",
+            ReadCommitted => "READ COMMITTED",
-            match self {
+            RepeatableRead => "REPEATABLE READ",
-                ReadUncommitted => "READ UNCOMMITTED",
+            Serializable => "SERIALIZABLE",
-                ReadCommitted => "READ COMMITTED",
+        })
-                RepeatableRead => "REPEATABLE READ",
-                Serializable => "SERIALIZABLE",
-            }
-        )
     }
 }

src/ast/operator.rs

@@ -22,15 +22,11 @@ pub enum UnaryOperator {
 
 impl fmt::Display for UnaryOperator {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(
+        f.write_str(match self {
-            f,
+            UnaryOperator::Plus => "+",
-            "{}",
+            UnaryOperator::Minus => "-",
-            match self {
+            UnaryOperator::Not => "NOT",
-                UnaryOperator::Plus => "+",
+        })
-                UnaryOperator::Minus => "-",
-                UnaryOperator::Not => "NOT",
-            }
-        )
     }
 }
 
@@ -56,26 +52,22 @@ pub enum BinaryOperator {
 
 impl fmt::Display for BinaryOperator {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(
+        f.write_str(match self {
-            f,
+            BinaryOperator::Plus => "+",
-            "{}",
+            BinaryOperator::Minus => "-",
-            match self {
+            BinaryOperator::Multiply => "*",
-                BinaryOperator::Plus => "+",
+            BinaryOperator::Divide => "/",
-                BinaryOperator::Minus => "-",
+            BinaryOperator::Modulus => "%",
-                BinaryOperator::Multiply => "*",
+            BinaryOperator::Gt => ">",
-                BinaryOperator::Divide => "/",
+            BinaryOperator::Lt => "<",
-                BinaryOperator::Modulus => "%",
+            BinaryOperator::GtEq => ">=",
-                BinaryOperator::Gt => ">",
+            BinaryOperator::LtEq => "<=",
-                BinaryOperator::Lt => "<",
+            BinaryOperator::Eq => "=",
-                BinaryOperator::GtEq => ">=",
+            BinaryOperator::NotEq => "<>",
-                BinaryOperator::LtEq => "<=",
+            BinaryOperator::And => "AND",
-                BinaryOperator::Eq => "=",
+            BinaryOperator::Or => "OR",
-                BinaryOperator::NotEq => "<>",
+            BinaryOperator::Like => "LIKE",
-                BinaryOperator::And => "AND",
+            BinaryOperator::NotLike => "NOT LIKE",
-                BinaryOperator::Or => "OR",
+        })
-                BinaryOperator::Like => "LIKE",
-                BinaryOperator::NotLike => "NOT LIKE",
-            }
-        )
     }
 }

src/ast/query.rs

@@ -100,15 +100,11 @@ pub enum SetOperator {
 
 impl fmt::Display for SetOperator {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(
+        f.write_str(match self {
-            f,
+            SetOperator::Union => "UNION",
-            "{}",
+            SetOperator::Except => "EXCEPT",
-            match self {
+            SetOperator::Intersect => "INTERSECT",
-                SetOperator::Union => "UNION",
+        })
-                SetOperator::Except => "EXCEPT",
-                SetOperator::Intersect => "INTERSECT",
-            }
-        )
     }
 }
 

src/ast/value.rs

@@ -128,18 +128,14 @@ pub enum DateTimeField {
 
 impl fmt::Display for DateTimeField {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(
+        f.write_str(match self {
-            f,
+            DateTimeField::Year => "YEAR",
-            "{}",
+            DateTimeField::Month => "MONTH",
-            match self {
+            DateTimeField::Day => "DAY",
-                DateTimeField::Year => "YEAR",
+            DateTimeField::Hour => "HOUR",
-                DateTimeField::Month => "MONTH",
+            DateTimeField::Minute => "MINUTE",
-                DateTimeField::Day => "DAY",
+            DateTimeField::Second => "SECOND",
-                DateTimeField::Hour => "HOUR",
+        })
-                DateTimeField::Minute => "MINUTE",
-                DateTimeField::Second => "SECOND",
-            }
-        )
     }
 }
 

src/parser.rs

@@ -285,16 +285,22 @@ impl Parser {
             self.expect_token(&Token::LParen)?;
             let partition_by = if self.parse_keywords(vec!["PARTITION", "BY"]) {
                 // a list of possibly-qualified column names
-                self.parse_expr_list()?
+                self.parse_comma_separated(Parser::parse_expr)?
             } else {
                 vec![]
             };
             let order_by = if self.parse_keywords(vec!["ORDER", "BY"]) {
-                self.parse_order_by_expr_list()?
+                self.parse_comma_separated(Parser::parse_order_by_expr)?
             } else {
                 vec![]
             };
-            let window_frame = self.parse_window_frame()?;
+            let window_frame = if !self.consume_token(&Token::RParen) {
+                let window_frame = self.parse_window_frame()?;
+                self.expect_token(&Token::RParen)?;
+                Some(window_frame)
+            } else {
+                None
+            };
 
             Some(WindowSpec {
                 partition_by,
@@ -313,38 +319,27 @@ impl Parser {
         }))
     }
 
-    pub fn parse_window_frame(&mut self) -> Result<Option<WindowFrame>, ParserError> {
+    pub fn parse_window_frame(&mut self) -> Result<WindowFrame, ParserError> {
-        let window_frame = match self.peek_token() {
+        let units = match self.next_token() {
-            Some(Token::Word(w)) => {
+            Some(Token::Word(w)) => w.keyword.parse::<WindowFrameUnits>()?,
-                let units = w.keyword.parse::<WindowFrameUnits>()?;
+            unexpected => return self.expected("ROWS, RANGE, GROUPS", unexpected),
-                self.next_token();
-                if self.parse_keyword("BETWEEN") {
-                    let start_bound = self.parse_window_frame_bound()?;
-                    self.expect_keyword("AND")?;
-                    let end_bound = Some(self.parse_window_frame_bound()?);
-                    Some(WindowFrame {
-                        units,
-                        start_bound,
-                        end_bound,
-                    })
-                } else {
-                    let start_bound = self.parse_window_frame_bound()?;
-                    let end_bound = None;
-                    Some(WindowFrame {
-                        units,
-                        start_bound,
-                        end_bound,
-                    })
-                }
-            }
-            Some(Token::RParen) => None,
-            unexpected => return self.expected("'ROWS', 'RANGE', 'GROUPS', or ')'", unexpected),
         };
-        self.expect_token(&Token::RParen)?;
+        let (start_bound, end_bound) = if self.parse_keyword("BETWEEN") {
-        Ok(window_frame)
+            let start_bound = self.parse_window_frame_bound()?;
+            self.expect_keyword("AND")?;
+            let end_bound = Some(self.parse_window_frame_bound()?);
+            (start_bound, end_bound)
+        } else {
+            (self.parse_window_frame_bound()?, None)
+        };
+        Ok(WindowFrame {
+            units,
+            start_bound,
+            end_bound,
+        })
     }
 
-    /// "CURRENT ROW" | ( (<positive number> | "UNBOUNDED") ("PRECEDING" | FOLLOWING) )
+    /// Parse `CURRENT ROW` or `{ <positive number> | UNBOUNDED } { PRECEDING | FOLLOWING }`
     pub fn parse_window_frame_bound(&mut self) -> Result<WindowFrameBound, ParserError> {
         if self.parse_keywords(vec!["CURRENT", "ROW"]) {
             Ok(WindowFrameBound::CurrentRow)
@@ -352,8 +347,7 @@ impl Parser {
             let rows = if self.parse_keyword("UNBOUNDED") {
                 None
             } else {
-                let rows = self.parse_literal_uint()?;
+                Some(self.parse_literal_uint()?)
-                Some(rows)
             };
             if self.parse_keyword("PRECEDING") {
                 Ok(WindowFrameBound::Preceding(rows))
@@ -597,7 +591,7 @@ impl Parser {
         } else {
             Expr::InList {
                 expr: Box::new(expr),
-                list: self.parse_expr_list()?,
+                list: self.parse_comma_separated(Parser::parse_expr)?,
                 negated,
             }
         };
@@ -829,6 +823,21 @@ impl Parser {
         }
     }
 
+    /// Parse a comma-separated list of 1+ items accepted by `F`
+    pub fn parse_comma_separated<T, F>(&mut self, mut f: F) -> Result<Vec<T>, ParserError>
+    where
+        F: FnMut(&mut Parser) -> Result<T, ParserError>,
+    {
+        let mut values = vec![];
+        loop {
+            values.push(f(self)?);
+            if !self.consume_token(&Token::Comma) {
+                break;
+            }
+        }
+        Ok(values)
+    }
+
     /// Parse a SQL CREATE statement
     pub fn parse_create(&mut self) -> Result<Statement, ParserError> {
         if self.parse_keyword("TABLE") {
@@ -872,11 +881,7 @@ impl Parser {
         // ANSI SQL and Postgres support RECURSIVE here, but we don't support it either.
         let name = self.parse_object_name()?;
         let columns = self.parse_parenthesized_column_list(Optional)?;
-        let with_options = if self.parse_keyword("WITH") {
+        let with_options = self.parse_with_options()?;
-            self.parse_with_options()?
-        } else {
-            vec![]
-        };
         self.expect_keyword("AS")?;
         let query = Box::new(self.parse_query()?);
         // Optional `WITH [ CASCADED | LOCAL ] CHECK OPTION` is widely supported here.
@@ -897,14 +902,10 @@ impl Parser {
         } else {
             return self.expected("TABLE or VIEW after DROP", self.peek_token());
         };
+        // Many dialects support the non standard `IF EXISTS` clause and allow
+        // specifying multiple objects to delete in a single statement
         let if_exists = self.parse_keywords(vec!["IF", "EXISTS"]);
-        let mut names = vec![];
+        let names = self.parse_comma_separated(Parser::parse_object_name)?;
-        loop {
-            names.push(self.parse_object_name()?);
-            if !self.consume_token(&Token::Comma) {
-                break;
-            }
-        }
         let cascade = self.parse_keyword("CASCADE");
         let restrict = self.parse_keyword("RESTRICT");
         if cascade && restrict {
@@ -922,12 +923,7 @@ impl Parser {
         let table_name = self.parse_object_name()?;
         // parse optional column list (schema)
         let (columns, constraints) = self.parse_columns()?;
-
+        let with_options = self.parse_with_options()?;
-        let with_options = if self.parse_keyword("WITH") {
-            self.parse_with_options()?
-        } else {
-            vec![]
-        };
 
         Ok(Statement::CreateTable {
             name: table_name,
@@ -1075,19 +1071,21 @@ impl Parser {
     }
 
     pub fn parse_with_options(&mut self) -> Result<Vec<SqlOption>, ParserError> {
-        self.expect_token(&Token::LParen)?;
+        if self.parse_keyword("WITH") {
-        let mut options = vec![];
+            self.expect_token(&Token::LParen)?;
-        loop {
+            let options = self.parse_comma_separated(Parser::parse_sql_option)?;
-            let name = self.parse_identifier()?;
+            self.expect_token(&Token::RParen)?;
-            self.expect_token(&Token::Eq)?;
+            Ok(options)
-            let value = self.parse_value()?;
+        } else {
-            options.push(SqlOption { name, value });
+            Ok(vec![])
-            if !self.consume_token(&Token::Comma) {
-                break;
-            }
         }
-        self.expect_token(&Token::RParen)?;
+    }
-        Ok(options)
+
+    pub fn parse_sql_option(&mut self) -> Result<SqlOption, ParserError> {
+        let name = self.parse_identifier()?;
+        self.expect_token(&Token::Eq)?;
+        let value = self.parse_value()?;
+        Ok(SqlOption { name, value })
     }
 
     pub fn parse_alter(&mut self) -> Result<Statement, ParserError> {
@@ -1333,22 +1331,17 @@ impl Parser {
         }
     }
 
-    /// Parse one or more identifiers with the specified separator between them
-    pub fn parse_list_of_ids(&mut self, separator: &Token) -> Result<Vec<Ident>, ParserError> {
-        let mut idents = vec![];
-        loop {
-            idents.push(self.parse_identifier()?);
-            if !self.consume_token(separator) {
-                break;
-            }
-        }
-        Ok(idents)
-    }
-
     /// Parse a possibly qualified, possibly quoted identifier, e.g.
     /// `foo` or `myschema."table"`
     pub fn parse_object_name(&mut self) -> Result<ObjectName, ParserError> {
-        Ok(ObjectName(self.parse_list_of_ids(&Token::Period)?))
+        let mut idents = vec![];
+        loop {
+            idents.push(self.parse_identifier()?);
+            if !self.consume_token(&Token::Period) {
+                break;
+            }
+        }
+        Ok(ObjectName(idents))
     }
 
     /// Parse a simple one-word identifier (possibly quoted, possibly a keyword)
@@ -1365,7 +1358,7 @@ impl Parser {
         optional: IsOptional,
     ) -> Result<Vec<Ident>, ParserError> {
         if self.consume_token(&Token::LParen) {
-            let cols = self.parse_list_of_ids(&Token::Comma)?;
+            let cols = self.parse_comma_separated(Parser::parse_identifier)?;
             self.expect_token(&Token::RParen)?;
             Ok(cols)
         } else if optional == Optional {
@@ -1424,7 +1417,7 @@ impl Parser {
     pub fn parse_query(&mut self) -> Result<Query, ParserError> {
         let ctes = if self.parse_keyword("WITH") {
             // TODO: optional RECURSIVE
-            self.parse_cte_list()?
+            self.parse_comma_separated(Parser::parse_cte)?
         } else {
             vec![]
         };
@@ -1432,7 +1425,7 @@ impl Parser {
         let body = self.parse_query_body(0)?;
 
         let order_by = if self.parse_keywords(vec!["ORDER", "BY"]) {
-            self.parse_order_by_expr_list()?
+            self.parse_comma_separated(Parser::parse_order_by_expr)?
         } else {
             vec![]
         };
@@ -1465,27 +1458,17 @@ impl Parser {
         })
     }
 
-    /// Parse one or more (comma-separated) `alias AS (subquery)` CTEs,
+    /// Parse a CTE (`alias [( col1, col2, ... )] AS (subquery)`)
-    /// assuming the initial `WITH` was already consumed.
+    fn parse_cte(&mut self) -> Result<Cte, ParserError> {
-    fn parse_cte_list(&mut self) -> Result<Vec<Cte>, ParserError> {
+        let alias = TableAlias {
-        let mut cte = vec![];
+            name: self.parse_identifier()?,
-        loop {
+            columns: self.parse_parenthesized_column_list(Optional)?,
-            let alias = TableAlias {
+        };
-                name: self.parse_identifier()?,
+        self.expect_keyword("AS")?;
-                columns: self.parse_parenthesized_column_list(Optional)?,
+        self.expect_token(&Token::LParen)?;
-            };
+        let query = self.parse_query()?;
-            self.expect_keyword("AS")?;
+        self.expect_token(&Token::RParen)?;
-            self.expect_token(&Token::LParen)?;
+        Ok(Cte { alias, query })
-            cte.push(Cte {
-                alias,
-                query: self.parse_query()?,
-            });
-            self.expect_token(&Token::RParen)?;
-            if !self.consume_token(&Token::Comma) {
-                break;
-            }
-        }
-        Ok(cte)
     }
 
     /// Parse a "query body", which is an expression with roughly the
@@ -1559,22 +1542,18 @@ impl Parser {
         if all && distinct {
             return parser_err!("Cannot specify both ALL and DISTINCT in SELECT");
         }
-        let projection = self.parse_select_list()?;
+        let projection = self.parse_comma_separated(Parser::parse_select_item)?;
 
         // 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![];
+        let from = if self.parse_keyword("FROM") {
-        if self.parse_keyword("FROM") {
+            self.parse_comma_separated(Parser::parse_table_and_joins)?
-            loop {
+        } else {
-                from.push(self.parse_table_and_joins()?);
+            vec![]
-                if !self.consume_token(&Token::Comma) {
+        };
-                    break;
-                }
-            }
-        }
 
         let selection = if self.parse_keyword("WHERE") {
             Some(self.parse_expr()?)
@@ -1583,7 +1562,7 @@ impl Parser {
         };
 
         let group_by = if self.parse_keywords(vec!["GROUP", "BY"]) {
-            self.parse_expr_list()?
+            self.parse_comma_separated(Parser::parse_expr)?
         } else {
             vec![]
         };
@@ -1749,7 +1728,7 @@ impl Parser {
             let mut with_hints = vec![];
             if self.parse_keyword("WITH") {
                 if self.consume_token(&Token::LParen) {
-                    with_hints = self.parse_expr_list()?;
+                    with_hints = self.parse_comma_separated(Parser::parse_expr)?;
                     self.expect_token(&Token::RParen)?;
                 } else {
                     // rewind, as WITH may belong to the next statement's CTE
@@ -1812,16 +1791,7 @@ impl Parser {
     pub fn parse_update(&mut self) -> Result<Statement, ParserError> {
         let table_name = self.parse_object_name()?;
         self.expect_keyword("SET")?;
-        let mut assignments = vec![];
+        let assignments = self.parse_comma_separated(Parser::parse_assignment)?;
-        loop {
-            let id = self.parse_identifier()?;
-            self.expect_token(&Token::Eq)?;
-            let value = self.parse_expr()?;
-            assignments.push(Assignment { id, value });
-            if !self.consume_token(&Token::Comma) {
-                break;
-            }
-        }
         let selection = if self.parse_keyword("WHERE") {
             Some(self.parse_expr()?)
         } else {
@@ -1834,75 +1804,53 @@ impl Parser {
         })
     }
 
-    /// Parse a comma-delimited list of SQL expressions
+    /// Parse a `var = expr` assignment, used in an UPDATE statement
-    pub fn parse_expr_list(&mut self) -> Result<Vec<Expr>, ParserError> {
+    pub fn parse_assignment(&mut self) -> Result<Assignment, ParserError> {
-        let mut expr_list: Vec<Expr> = vec![];
+        let id = self.parse_identifier()?;
-        loop {
+        self.expect_token(&Token::Eq)?;
-            expr_list.push(self.parse_expr()?);
+        let value = self.parse_expr()?;
-            if !self.consume_token(&Token::Comma) {
+        Ok(Assignment { id, value })
-                break;
-            }
-        }
-        Ok(expr_list)
     }
 
     pub fn parse_optional_args(&mut self) -> Result<Vec<Expr>, ParserError> {
         if self.consume_token(&Token::RParen) {
             Ok(vec![])
         } else {
-            let args = self.parse_expr_list()?;
+            let args = self.parse_comma_separated(Parser::parse_expr)?;
             self.expect_token(&Token::RParen)?;
             Ok(args)
         }
     }
 
     /// Parse a comma-delimited list of projections after SELECT
-    pub fn parse_select_list(&mut self) -> Result<Vec<SelectItem>, ParserError> {
+    pub fn parse_select_item(&mut self) -> Result<SelectItem, ParserError> {
-        let mut projections: Vec<SelectItem> = vec![];
+        let expr = self.parse_expr()?;
-        loop {
+        if let Expr::Wildcard = expr {
-            let expr = self.parse_expr()?;
+            Ok(SelectItem::Wildcard)
-            if let Expr::Wildcard = expr {
+        } else if let Expr::QualifiedWildcard(prefix) = expr {
-                projections.push(SelectItem::Wildcard);
+            Ok(SelectItem::QualifiedWildcard(ObjectName(prefix)))
-            } else if let Expr::QualifiedWildcard(prefix) = expr {
+        } else {
-                projections.push(SelectItem::QualifiedWildcard(ObjectName(prefix)));
+            // `expr` is a regular SQL expression and can be followed by an alias
+            if let Some(alias) = self.parse_optional_alias(keywords::RESERVED_FOR_COLUMN_ALIAS)? {
+                Ok(SelectItem::ExprWithAlias { expr, alias })
             } else {
-                // `expr` is a regular SQL expression and can be followed by an alias
+                Ok(SelectItem::UnnamedExpr(expr))
-                if let Some(alias) =
-                    self.parse_optional_alias(keywords::RESERVED_FOR_COLUMN_ALIAS)?
-                {
-                    projections.push(SelectItem::ExprWithAlias { expr, alias });
-                } else {
-                    projections.push(SelectItem::UnnamedExpr(expr));
-                }
-            }
-
-            if !self.consume_token(&Token::Comma) {
-                break;
             }
         }
-        Ok(projections)
     }
 
-    /// Parse a comma-delimited list of ORDER BY expressions
+    /// Parse an expression, optionally followed by ASC or DESC (used in ORDER BY)
-    pub fn parse_order_by_expr_list(&mut self) -> Result<Vec<OrderByExpr>, ParserError> {
+    pub fn parse_order_by_expr(&mut self) -> Result<OrderByExpr, ParserError> {
-        let mut expr_list: Vec<OrderByExpr> = vec![];
+        let expr = self.parse_expr()?;
-        loop {
-            let expr = self.parse_expr()?;
 
-            let asc = if self.parse_keyword("ASC") {
+        let asc = if self.parse_keyword("ASC") {
-                Some(true)
+            Some(true)
-            } else if self.parse_keyword("DESC") {
+        } else if self.parse_keyword("DESC") {
-                Some(false)
+            Some(false)
-            } else {
+        } else {
-                None
+            None
-            };
+        };
-
+        Ok(OrderByExpr { expr, asc })
-            expr_list.push(OrderByExpr { expr, asc });
-            if !self.consume_token(&Token::Comma) {
-                break;
-            }
-        }
-        Ok(expr_list)
     }
 
     /// Parse a LIMIT clause
@@ -1950,15 +1898,12 @@ impl Parser {
     }
 
     pub fn parse_values(&mut self) -> Result<Values, ParserError> {
-        let mut values = vec![];
+        let values = self.parse_comma_separated(|parser| {
-        loop {
+            parser.expect_token(&Token::LParen)?;
-            self.expect_token(&Token::LParen)?;
+            let exprs = parser.parse_comma_separated(Parser::parse_expr)?;
-            values.push(self.parse_expr_list()?);
+            parser.expect_token(&Token::RParen)?;
-            self.expect_token(&Token::RParen)?;
+            Ok(exprs)
-            if !self.consume_token(&Token::Comma) {
+        })?;
-                break;
-            }
-        }
         Ok(Values(values))
     }