From 6896953f1a4ccfb1a680648b3188f451fa8699ec Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 06:34:38 -0700 Subject: [PATCH 01/29] provide LISTAGG implementation This patch provides an initial implemenation of LISTAGG[1]. Notably this implemenation deviates from ANSI SQL by allowing both WITHIN GROUP and the delimiter to be optional. We do so because Redshift SQL works this way and this approach is ultimately more flexible. Fixes #169. [1] https://modern-sql.com/feature/listagg --- src/ast/mod.rs | 68 ++++++++++++++++++++++++++++++++++++++ src/dialect/keywords.rs | 3 ++ src/parser.rs | 69 +++++++++++++++++++++++++++++++++++++++ tests/sqlparser_common.rs | 51 +++++++++++++++++++++++++++++ 4 files changed, 191 insertions(+) diff --git a/src/ast/mod.rs b/src/ast/mod.rs index a867abcf9..45447fb2c 100644 --- a/src/ast/mod.rs +++ b/src/ast/mod.rs @@ -224,6 +224,8 @@ pub enum Expr { /// A parenthesized subquery `(SELECT ...)`, used in expression like /// `SELECT (subquery) AS x` or `WHERE (subquery) = x` Subquery(Box), + /// The `lISTAGG` function, e.g. `SELECT LISTAGG(...) WITHIN GROUP (ORDER BY ...)`. + ListAgg(ListAgg), } impl fmt::Display for Expr { @@ -299,6 +301,7 @@ impl fmt::Display for Expr { } Expr::Exists(s) => write!(f, "EXISTS ({})", s), Expr::Subquery(s) => write!(f, "({})", s), + Expr::ListAgg(listagg) => write!(f, "{}", listagg), } } } @@ -850,6 +853,71 @@ impl FromStr for FileFormat { } } +/// A `LISTAGG` invocation +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct ListAgg { + pub distinct: bool, + pub expr: Box, + pub separator: Option>, + pub on_overflow: Option, + pub within_group: Option>, +} + +impl fmt::Display for ListAgg { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let distinct = if self.distinct { "DISTINCT " } else { "ALL " }; + let args = if let Some(separator) = &self.separator { + format!( + "{}", + display_comma_separated(&[self.expr.clone(), separator.clone()]) + ) + } else { + format!("{}", self.expr) + }; + if let Some(on_overflow) = &self.on_overflow { + write!(f, "LISTAGG({}{}{})", distinct, args, on_overflow) + } else { + write!(f, "LISTAGG({}{})", distinct, args) + }?; + if let Some(within_group) = &self.within_group { + write!( + f, + " WITHIN GROUP (ORDER BY {})", + display_comma_separated(within_group) + ) + } else { + Ok(()) + } + } +} + +/// The `ON OVERFLOW` clause of a LISTAGG invocation +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct ListAggOnOverflow { + pub error: bool, + pub filler: Option>, + pub with_count: bool, +} + +impl fmt::Display for ListAggOnOverflow { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let overflow = if self.error { + "ERROR".to_owned() + } else { + format!( + "TRUNCATE {}{} COUNT", + if let Some(filler) = &self.filler { + format!("{} ", filler) + } else { + "".to_owned() + }, + if self.with_count { "WITH" } else { "WITHOUT" } + ) + }; + write!(f, " ON OVERFLOW {}", overflow) + } +} + #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum ObjectType { Table, diff --git a/src/dialect/keywords.rs b/src/dialect/keywords.rs index a01871c6e..ee59a1c92 100644 --- a/src/dialect/keywords.rs +++ b/src/dialect/keywords.rs @@ -161,6 +161,7 @@ define_keywords!( END_FRAME, END_PARTITION, EQUALS, + ERROR, ESCAPE, EVERY, EXCEPT, @@ -230,6 +231,7 @@ define_keywords!( LIKE, LIKE_REGEX, LIMIT, + LISTAGG, LN, LOCAL, LOCALTIME, @@ -279,6 +281,7 @@ define_keywords!( OUT, OUTER, OVER, + OVERFLOW, OVERLAPS, OVERLAY, PARAMETER, diff --git a/src/parser.rs b/src/parser.rs index 608ac4736..ac4da095e 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -191,6 +191,7 @@ impl Parser { "EXISTS" => self.parse_exists_expr(), "EXTRACT" => self.parse_extract_expr(), "INTERVAL" => self.parse_literal_interval(), + "LISTAGG" => self.parse_listagg_expr(), "NOT" => Ok(Expr::UnaryOp { op: UnaryOperator::Not, expr: Box::new(self.parse_subexpr(Self::UNARY_NOT_PREC)?), @@ -423,6 +424,74 @@ impl Parser { }) } + /// Parse a SQL LISTAGG expression, e.g. `LISTAGG(...) WITHIN GROUP (ORDER BY ...)`. + pub fn parse_listagg_expr(&mut self) -> Result { + self.expect_token(&Token::LParen)?; + let all = self.parse_keyword("ALL"); + let distinct = self.parse_keyword("DISTINCT"); + if all && distinct { + return parser_err!("Cannot specify both ALL and DISTINCT in LISTAGG".to_string()); + } + let args = self.parse_comma_separated(Parser::parse_expr)?; + // TODO: Is there a safer way of grabbing the expr and separator? + let expr = Box::new(args[0].clone()); + // While ANSI SQL would require the separator, Redshift makes this optional. Here we also + // make the separator optional as this provides a more general implementation. + let separator = if let Some(separtor) = args.get(1) { + Some(Box::new(separtor.clone())) + } else { + None + }; + let on_overflow = if self.parse_keywords(vec!["ON", "OVERFLOW"]) { + let error = self.parse_keyword("ERROR"); + let filler = if !error { + self.expect_keyword("TRUNCATE")?; + Some(Box::new(self.parse_expr()?)) + } else { + None + }; + let with_count = if !error { + let with_count = self.parse_keywords(vec!["WITH", "COUNT"]); + let without_count = self.parse_keywords(vec!["WITHOUT", "COUNT"]); + if !with_count && !without_count { + return parser_err!( + "Expected either WITH COUNT or WITHOUT COUNT in LISTAGG".to_string() + ); + }; + with_count + } else { + false + }; + Some(ListAggOnOverflow { + error, + filler, + with_count, + }) + } else { + None + }; + self.expect_token(&Token::RParen)?; + // Once again ANSI SQL requires WITHIN GROUP, but Redshift does not. Again we choose the + // more general implementation. + let within_group = if self.parse_keywords(vec!["WITHIN", "GROUP"]) { + self.expect_token(&Token::LParen)?; + self.expect_keywords(&["ORDER", "BY"])?; + Some(self.parse_comma_separated(Parser::parse_order_by_expr)?) + } else { + None + }; + if within_group.is_some() { + self.expect_token(&Token::RParen)?; + } + Ok(Expr::ListAgg(ListAgg { + distinct, + expr, + separator, + on_overflow, + within_group, + })) + } + // This function parses date/time fields for both the EXTRACT function-like // operator and interval qualifiers. EXTRACT supports a wider set of // date/time fields than interval qualifiers, so this function may need to diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index c87fcf3a2..b65216cce 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -914,6 +914,57 @@ fn parse_extract() { ); } +#[test] +fn parse_listagg() { + let sql = "SELECT LISTAGG(DISTINCT dateid, ', ' ON OVERFLOW TRUNCATE '%' WITHOUT COUNT) \ + WITHIN GROUP (ORDER BY id, username)"; + let select = verified_only_select(sql); + + one_statement_parses_to( + "SELECT LISTAGG(sellerid) WITHIN GROUP (ORDER BY dateid)", + "SELECT LISTAGG(ALL sellerid) WITHIN GROUP (ORDER BY dateid)", + ); + one_statement_parses_to("SELECT LISTAGG(dateid)", "SELECT LISTAGG(ALL dateid)"); + verified_stmt("SELECT LISTAGG(DISTINCT dateid)"); + + let expr = Box::new(Expr::Identifier(Ident::new("dateid"))); + let on_overflow = Some(ListAggOnOverflow { + error: false, + filler: Some(Box::new(Expr::Value(Value::SingleQuotedString( + "%".to_string(), + )))), + with_count: false, + }); + let within_group = Some(vec![ + OrderByExpr { + expr: Expr::Identifier(Ident { + value: "id".to_string(), + quote_style: None, + }), + asc: None, + }, + OrderByExpr { + expr: Expr::Identifier(Ident { + value: "username".to_string(), + quote_style: None, + }), + asc: None, + }, + ]); + assert_eq!( + &Expr::ListAgg(ListAgg { + distinct: true, + expr, + separator: Some(Box::new(Expr::Value(Value::SingleQuotedString( + ", ".to_string() + )))), + on_overflow, + within_group + }), + expr_from_projection(only(&select.projection)) + ); +} + #[test] fn parse_create_table() { let sql = "CREATE TABLE uk_cities (\ From c6216ec03bdaa35918866483552aae40b7dd2348 Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 15:38:13 -0700 Subject: [PATCH 02/29] provide a reminder for LISTAGG syntax --- src/ast/mod.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/ast/mod.rs b/src/ast/mod.rs index 45447fb2c..dee9cb31f 100644 --- a/src/ast/mod.rs +++ b/src/ast/mod.rs @@ -853,7 +853,9 @@ impl FromStr for FileFormat { } } -/// A `LISTAGG` invocation +/// A `LISTAGG` invocation, e.g. +/// LISTAGG( [ DISTINCT ] [, ] [ON OVERFLOW ] ) +/// [ WITHIN GROUP (ORDER BY [, ...] ) ] #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct ListAgg { pub distinct: bool, From 48d6a8b92bc66c9767a8722726a178c507307f4d Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 15:42:14 -0700 Subject: [PATCH 03/29] prefer Vec over Option> --- src/ast/mod.rs | 6 +++--- src/parser.rs | 6 +++--- tests/sqlparser_common.rs | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/ast/mod.rs b/src/ast/mod.rs index dee9cb31f..422e592ee 100644 --- a/src/ast/mod.rs +++ b/src/ast/mod.rs @@ -862,7 +862,7 @@ pub struct ListAgg { pub expr: Box, pub separator: Option>, pub on_overflow: Option, - pub within_group: Option>, + pub within_group: Vec, } impl fmt::Display for ListAgg { @@ -881,11 +881,11 @@ impl fmt::Display for ListAgg { } else { write!(f, "LISTAGG({}{})", distinct, args) }?; - if let Some(within_group) = &self.within_group { + if !self.within_group.is_empty() { write!( f, " WITHIN GROUP (ORDER BY {})", - display_comma_separated(within_group) + display_comma_separated(&self.within_group) ) } else { Ok(()) diff --git a/src/parser.rs b/src/parser.rs index ac4da095e..a80ac4eea 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -476,11 +476,11 @@ impl Parser { let within_group = if self.parse_keywords(vec!["WITHIN", "GROUP"]) { self.expect_token(&Token::LParen)?; self.expect_keywords(&["ORDER", "BY"])?; - Some(self.parse_comma_separated(Parser::parse_order_by_expr)?) + self.parse_comma_separated(Parser::parse_order_by_expr)? } else { - None + vec![] }; - if within_group.is_some() { + if !within_group.is_empty() { self.expect_token(&Token::RParen)?; } Ok(Expr::ListAgg(ListAgg { diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index b65216cce..e97bf794d 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -935,7 +935,7 @@ fn parse_listagg() { )))), with_count: false, }); - let within_group = Some(vec![ + let within_group = vec![ OrderByExpr { expr: Expr::Identifier(Ident { value: "id".to_string(), @@ -950,7 +950,7 @@ fn parse_listagg() { }), asc: None, }, - ]); + ]; assert_eq!( &Expr::ListAgg(ListAgg { distinct: true, From eba849a4872c9fe006edd60fd0492ae7edd73af8 Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 15:44:55 -0700 Subject: [PATCH 04/29] ensure we do not explicitly print implied `ALL` --- src/ast/mod.rs | 2 +- tests/sqlparser_common.rs | 7 ++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/ast/mod.rs b/src/ast/mod.rs index 422e592ee..5635117ee 100644 --- a/src/ast/mod.rs +++ b/src/ast/mod.rs @@ -867,7 +867,7 @@ pub struct ListAgg { impl fmt::Display for ListAgg { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let distinct = if self.distinct { "DISTINCT " } else { "ALL " }; + let distinct = if self.distinct { "DISTINCT " } else { "" }; let args = if let Some(separator) = &self.separator { format!( "{}", diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index e97bf794d..9fb380471 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -920,11 +920,8 @@ fn parse_listagg() { WITHIN GROUP (ORDER BY id, username)"; let select = verified_only_select(sql); - one_statement_parses_to( - "SELECT LISTAGG(sellerid) WITHIN GROUP (ORDER BY dateid)", - "SELECT LISTAGG(ALL sellerid) WITHIN GROUP (ORDER BY dateid)", - ); - one_statement_parses_to("SELECT LISTAGG(dateid)", "SELECT LISTAGG(ALL dateid)"); + verified_stmt("SELECT LISTAGG(sellerid) WITHIN GROUP (ORDER BY dateid)"); + verified_stmt("SELECT LISTAGG(dateid)"); verified_stmt("SELECT LISTAGG(DISTINCT dateid)"); let expr = Box::new(Expr::Identifier(Ident::new("dateid"))); From a95fa551d633eae5ad8b97d3399a7823b8b63f5a Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 16:10:00 -0700 Subject: [PATCH 05/29] refactor ListAggOnOverflow into an enum --- src/ast/mod.rs | 38 +++++++++++++++++++++----------------- src/parser.rs | 36 ++++++++++++++++-------------------- tests/sqlparser_common.rs | 3 +-- 3 files changed, 38 insertions(+), 39 deletions(-) diff --git a/src/ast/mod.rs b/src/ast/mod.rs index 5635117ee..3ac54b57b 100644 --- a/src/ast/mod.rs +++ b/src/ast/mod.rs @@ -895,28 +895,32 @@ impl fmt::Display for ListAgg { /// The `ON OVERFLOW` clause of a LISTAGG invocation #[derive(Debug, Clone, PartialEq, Eq, Hash)] -pub struct ListAggOnOverflow { - pub error: bool, - pub filler: Option>, - pub with_count: bool, +pub enum ListAggOnOverflow { + Error, + Truncate { + filler: Option>, + with_count: bool, + }, } impl fmt::Display for ListAggOnOverflow { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let overflow = if self.error { - "ERROR".to_owned() - } else { - format!( - "TRUNCATE {}{} COUNT", - if let Some(filler) = &self.filler { - format!("{} ", filler) + write!(f, " ON OVERFLOW")?; + match self { + ListAggOnOverflow::Error => write!(f, " ERROR"), + ListAggOnOverflow::Truncate { filler, with_count } => { + write!(f, " TRUNCATE")?; + if let Some(filler) = filler { + write!(f, " {}", filler)?; + } + if *with_count { + write!(f, " WITH")?; } else { - "".to_owned() - }, - if self.with_count { "WITH" } else { "WITHOUT" } - ) - }; - write!(f, " ON OVERFLOW {}", overflow) + write!(f, " WITHOUT")?; + } + write!(f, " COUNT") + } + } } } diff --git a/src/parser.rs b/src/parser.rs index a80ac4eea..92b4125fd 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -444,28 +444,24 @@ impl Parser { }; let on_overflow = if self.parse_keywords(vec!["ON", "OVERFLOW"]) { let error = self.parse_keyword("ERROR"); - let filler = if !error { - self.expect_keyword("TRUNCATE")?; - Some(Box::new(self.parse_expr()?)) + Some(if error { + ListAggOnOverflow::Error } else { - None - }; - let with_count = if !error { - let with_count = self.parse_keywords(vec!["WITH", "COUNT"]); - let without_count = self.parse_keywords(vec!["WITHOUT", "COUNT"]); - if !with_count && !without_count { - return parser_err!( - "Expected either WITH COUNT or WITHOUT COUNT in LISTAGG".to_string() - ); + self.expect_keyword("TRUNCATE")?; + let filler = Some(Box::new(self.parse_expr()?)); + let with_count = if !error { + let with_count = self.parse_keywords(vec!["WITH", "COUNT"]); + let without_count = self.parse_keywords(vec!["WITHOUT", "COUNT"]); + if !with_count && !without_count { + return parser_err!( + "Expected either WITH COUNT or WITHOUT COUNT in LISTAGG".to_string() + ); + }; + with_count + } else { + false }; - with_count - } else { - false - }; - Some(ListAggOnOverflow { - error, - filler, - with_count, + ListAggOnOverflow::Truncate { filler, with_count } }) } else { None diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 9fb380471..579a63016 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -925,8 +925,7 @@ fn parse_listagg() { verified_stmt("SELECT LISTAGG(DISTINCT dateid)"); let expr = Box::new(Expr::Identifier(Ident::new("dateid"))); - let on_overflow = Some(ListAggOnOverflow { - error: false, + let on_overflow = Some(ListAggOnOverflow::Truncate { filler: Some(Box::new(Expr::Value(Value::SingleQuotedString( "%".to_string(), )))), From ee144b2bae4b2974f8f512bd590ff200264566ab Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 16:25:50 -0700 Subject: [PATCH 06/29] provide parse_all_or_distinct abstraction This consolidates the act of checking for both `ALL` and `DISTINCT` into a parser function. Please note that this present implementation reduces the information provided by the resulting error message. This may be undesirable, so pending review, may certainly be updated or reverted. However it may also be the case that the additional contextual information is not necessary. --- src/parser.rs | 33 +++++++++++++++------------------ tests/sqlparser_common.rs | 4 ++-- 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/src/parser.rs b/src/parser.rs index 92b4125fd..fd81a046d 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -273,14 +273,7 @@ impl Parser { pub fn parse_function(&mut self, name: ObjectName) -> Result { self.expect_token(&Token::LParen)?; - let all = self.parse_keyword("ALL"); - let distinct = self.parse_keyword("DISTINCT"); - if all && distinct { - return parser_err!(format!( - "Cannot specify both ALL and DISTINCT in function: {}", - name.to_string(), - )); - } + let distinct = self.parse_all_or_distinct()?; let args = self.parse_optional_args()?; let over = if self.parse_keyword("OVER") { // TBD: support window names (`OVER mywin`) in place of inline specification @@ -427,11 +420,7 @@ impl Parser { /// Parse a SQL LISTAGG expression, e.g. `LISTAGG(...) WITHIN GROUP (ORDER BY ...)`. pub fn parse_listagg_expr(&mut self) -> Result { self.expect_token(&Token::LParen)?; - let all = self.parse_keyword("ALL"); - let distinct = self.parse_keyword("DISTINCT"); - if all && distinct { - return parser_err!("Cannot specify both ALL and DISTINCT in LISTAGG".to_string()); - } + let distinct = self.parse_all_or_distinct()?; let args = self.parse_comma_separated(Parser::parse_expr)?; // TODO: Is there a safer way of grabbing the expr and separator? let expr = Box::new(args[0].clone()); @@ -916,6 +905,18 @@ impl Parser { Ok(values) } + /// Parse either `ALL` or `DISTINCT`. Returns `true` if `DISTINCT` is parsed and results in a + /// `ParserError` if both `ALL` and `DISTINCT` are fround. + pub fn parse_all_or_distinct(&mut self) -> Result { + let all = self.parse_keyword("ALL"); + let distinct = self.parse_keyword("DISTINCT"); + if all && distinct { + return parser_err!("Cannot specify both ALL and DISTINCT".to_string()); + } else { + Ok(distinct) + } + } + /// Parse a SQL CREATE statement pub fn parse_create(&mut self) -> Result { if self.parse_keyword("TABLE") { @@ -1700,11 +1701,7 @@ impl Parser { /// Parse a restricted `SELECT` statement (no CTEs / `UNION` / `ORDER BY`), /// assuming the initial `SELECT` was already consumed pub fn parse_select(&mut self) -> Result { - let all = self.parse_keyword("ALL"); - let distinct = self.parse_keyword("DISTINCT"); - if all && distinct { - return parser_err!("Cannot specify both ALL and DISTINCT in SELECT"); - } + let distinct = self.parse_all_or_distinct()?; let top = if self.parse_keyword("TOP") { Some(self.parse_top()?) diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 579a63016..106e6ef95 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -244,7 +244,7 @@ fn parse_select_all() { fn parse_select_all_distinct() { let result = parse_sql_statements("SELECT ALL DISTINCT name FROM customer"); assert_eq!( - ParserError::ParserError("Cannot specify both ALL and DISTINCT in SELECT".to_string()), + ParserError::ParserError("Cannot specify both ALL and DISTINCT".to_string()), result.unwrap_err(), ); } @@ -358,7 +358,7 @@ fn parse_select_count_distinct() { let res = parse_sql_statements(sql); assert_eq!( ParserError::ParserError( - "Cannot specify both ALL and DISTINCT in function: COUNT".to_string() + "Cannot specify both ALL and DISTINCT".to_string() ), res.unwrap_err() ); From 1e41d30bc56ad83ce22dbedd1bd58ab9f7a2b4d2 Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 16:31:15 -0700 Subject: [PATCH 07/29] clean up expr and separator parsing a bit --- src/parser.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/parser.rs b/src/parser.rs index fd81a046d..445f4f89d 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -421,13 +421,9 @@ impl Parser { pub fn parse_listagg_expr(&mut self) -> Result { self.expect_token(&Token::LParen)?; let distinct = self.parse_all_or_distinct()?; - let args = self.parse_comma_separated(Parser::parse_expr)?; - // TODO: Is there a safer way of grabbing the expr and separator? - let expr = Box::new(args[0].clone()); - // While ANSI SQL would require the separator, Redshift makes this optional. Here we also - // make the separator optional as this provides a more general implementation. - let separator = if let Some(separtor) = args.get(1) { - Some(Box::new(separtor.clone())) + let expr = Box::new(self.parse_expr()?); + let separator = if self.consume_token(&Token::Comma) { + Some(Box::new(self.parse_expr()?)) } else { None }; From 1e952b6e07dce82bcaf6b3036066b9bf356eaae9 Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 16:33:49 -0700 Subject: [PATCH 08/29] combine token checking into its logical block --- src/parser.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/parser.rs b/src/parser.rs index 445f4f89d..1de2a433d 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -457,13 +457,12 @@ impl Parser { let within_group = if self.parse_keywords(vec!["WITHIN", "GROUP"]) { self.expect_token(&Token::LParen)?; self.expect_keywords(&["ORDER", "BY"])?; - self.parse_comma_separated(Parser::parse_order_by_expr)? + let order_by_expr = self.parse_comma_separated(Parser::parse_order_by_expr)?; + self.expect_token(&Token::RParen)?; + order_by_expr } else { vec![] }; - if !within_group.is_empty() { - self.expect_token(&Token::RParen)?; - } Ok(Expr::ListAgg(ListAgg { distinct, expr, From 5736b9d32a6cac28a7e9af4b04e0ea84b38171af Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 16:39:51 -0700 Subject: [PATCH 09/29] ensure cargo fmt --- tests/sqlparser_common.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 106e6ef95..8c9664fc3 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -357,9 +357,7 @@ fn parse_select_count_distinct() { let sql = "SELECT COUNT(ALL DISTINCT + x) FROM customer"; let res = parse_sql_statements(sql); assert_eq!( - ParserError::ParserError( - "Cannot specify both ALL and DISTINCT".to_string() - ), + ParserError::ParserError("Cannot specify both ALL and DISTINCT".to_string()), res.unwrap_err() ); } From bcc41a35c89d8cdfa3d313132fac03fbea70f1d2 Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 18:00:50 -0700 Subject: [PATCH 10/29] extend doc-comment with example --- src/ast/mod.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ast/mod.rs b/src/ast/mod.rs index 3ac54b57b..b66c2f9fd 100644 --- a/src/ast/mod.rs +++ b/src/ast/mod.rs @@ -893,7 +893,8 @@ impl fmt::Display for ListAgg { } } -/// The `ON OVERFLOW` clause of a LISTAGG invocation +/// The `ON OVERFLOW` clause of a LISTAGG invocation e.g. +/// ON OVERFLOW ERROR | TRUNCATE [ ] WITH[OUT] COUNT #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum ListAggOnOverflow { Error, From a16c3a43e87c29837924c90f6a5cfa9078a80daf Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 18:05:37 -0700 Subject: [PATCH 11/29] eliminate unnecessary error variable --- src/parser.rs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/parser.rs b/src/parser.rs index 1de2a433d..e71b51565 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -428,13 +428,11 @@ impl Parser { None }; let on_overflow = if self.parse_keywords(vec!["ON", "OVERFLOW"]) { - let error = self.parse_keyword("ERROR"); - Some(if error { - ListAggOnOverflow::Error + if self.parse_keyword("ERROR") { + Some(ListAggOnOverflow::Error) } else { self.expect_keyword("TRUNCATE")?; let filler = Some(Box::new(self.parse_expr()?)); - let with_count = if !error { let with_count = self.parse_keywords(vec!["WITH", "COUNT"]); let without_count = self.parse_keywords(vec!["WITHOUT", "COUNT"]); if !with_count && !without_count { @@ -442,12 +440,8 @@ impl Parser { "Expected either WITH COUNT or WITHOUT COUNT in LISTAGG".to_string() ); }; - with_count - } else { - false - }; - ListAggOnOverflow::Truncate { filler, with_count } - }) + Some(ListAggOnOverflow::Truncate { filler, with_count }) + } } else { None }; From e1ad011623b7f42b7288168e157c8b1148be68f4 Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 18:41:20 -0700 Subject: [PATCH 12/29] ensure filler is optional --- src/parser.rs | 27 +++++++++++++++++++-------- tests/sqlparser_common.rs | 1 + 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/parser.rs b/src/parser.rs index e71b51565..b0c2822d7 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -432,14 +432,25 @@ impl Parser { Some(ListAggOnOverflow::Error) } else { self.expect_keyword("TRUNCATE")?; - let filler = Some(Box::new(self.parse_expr()?)); - let with_count = self.parse_keywords(vec!["WITH", "COUNT"]); - let without_count = self.parse_keywords(vec!["WITHOUT", "COUNT"]); - if !with_count && !without_count { - return parser_err!( - "Expected either WITH COUNT or WITHOUT COUNT in LISTAGG".to_string() - ); - }; + let filler = match self.peek_token() { + Some(tok) => { + if tok == Token::make_keyword("WITH") + || tok == Token::make_keyword("WITHOUT") + { + None + } else { + Some(Box::new(self.parse_expr()?)) + } + } + None => None, + }; + let with_count = self.parse_keywords(vec!["WITH", "COUNT"]); + let without_count = self.parse_keywords(vec!["WITHOUT", "COUNT"]); + if !with_count && !without_count { + return parser_err!( + "Expected either WITH COUNT or WITHOUT COUNT in LISTAGG".to_string() + ); + }; Some(ListAggOnOverflow::Truncate { filler, with_count }) } } else { diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 8c9664fc3..9ff9bd769 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -921,6 +921,7 @@ fn parse_listagg() { verified_stmt("SELECT LISTAGG(sellerid) WITHIN GROUP (ORDER BY dateid)"); verified_stmt("SELECT LISTAGG(dateid)"); verified_stmt("SELECT LISTAGG(DISTINCT dateid)"); + verified_stmt("SELECT LISTAGG(dateid ON OVERFLOW ERROR)"); let expr = Box::new(Expr::Identifier(Ident::new("dateid"))); let on_overflow = Some(ListAggOnOverflow::Truncate { From 158c6507acb84c3cd9bdffb482fdc70e5d38d1fb Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 18:42:50 -0700 Subject: [PATCH 13/29] prefer format over generalized abstraction --- src/ast/mod.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/ast/mod.rs b/src/ast/mod.rs index b66c2f9fd..414ef4fbb 100644 --- a/src/ast/mod.rs +++ b/src/ast/mod.rs @@ -869,10 +869,7 @@ impl fmt::Display for ListAgg { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let distinct = if self.distinct { "DISTINCT " } else { "" }; let args = if let Some(separator) = &self.separator { - format!( - "{}", - display_comma_separated(&[self.expr.clone(), separator.clone()]) - ) + format!("{}, {}", self.expr, separator) } else { format!("{}", self.expr) }; From b2cb82290b1343f7b3e3bb0aaacf69ba6b5e1fe4 Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 18:45:17 -0700 Subject: [PATCH 14/29] shorten with_count binding --- src/parser.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/parser.rs b/src/parser.rs index b0c2822d7..04c429348 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -444,13 +444,12 @@ impl Parser { } None => None, }; - let with_count = self.parse_keywords(vec!["WITH", "COUNT"]); - let without_count = self.parse_keywords(vec!["WITHOUT", "COUNT"]); - if !with_count && !without_count { - return parser_err!( - "Expected either WITH COUNT or WITHOUT COUNT in LISTAGG".to_string() - ); - }; + + let with_count = self.parse_keyword("WITH"); + if !with_count && !self.parse_keyword("WITHOUT") { + self.expected("either WITH or WITHOUT in LISTAGG", self.peek_token())?; + } + self.expect_keyword("COUNT")?; Some(ListAggOnOverflow::Truncate { filler, with_count }) } } else { From b5040729bc5d41b018f4df0da5a9cbd2eb9122a8 Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 18:58:13 -0700 Subject: [PATCH 15/29] rework display for listagg --- src/ast/mod.rs | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/ast/mod.rs b/src/ast/mod.rs index 414ef4fbb..fefd65501 100644 --- a/src/ast/mod.rs +++ b/src/ast/mod.rs @@ -867,17 +867,21 @@ pub struct ListAgg { impl fmt::Display for ListAgg { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let distinct = if self.distinct { "DISTINCT " } else { "" }; - let args = if let Some(separator) = &self.separator { - format!("{}, {}", self.expr, separator) - } else { - format!("{}", self.expr) - }; - if let Some(on_overflow) = &self.on_overflow { - write!(f, "LISTAGG({}{}{})", distinct, args, on_overflow) - } else { - write!(f, "LISTAGG({}{})", distinct, args) - }?; + write!( + f, + "LISTAGG({}{}{})", + if self.distinct { "DISTINCT " } else { "" }, + if let Some(separator) = &self.separator { + format!("{}, {}", self.expr, separator) + } else { + format!("{}", self.expr) + }, + if let Some(on_overflow) = &self.on_overflow { + format!("{}", on_overflow) + } else { + "".to_string() + } + )?; if !self.within_group.is_empty() { write!( f, From 7e53d375c7544693581f2568c2dbec9091008483 Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 19:02:56 -0700 Subject: [PATCH 16/29] ensure enum variants have proper doc-comments --- src/ast/mod.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/ast/mod.rs b/src/ast/mod.rs index fefd65501..9d96549f9 100644 --- a/src/ast/mod.rs +++ b/src/ast/mod.rs @@ -895,10 +895,12 @@ impl fmt::Display for ListAgg { } /// The `ON OVERFLOW` clause of a LISTAGG invocation e.g. -/// ON OVERFLOW ERROR | TRUNCATE [ ] WITH[OUT] COUNT #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum ListAggOnOverflow { + /// e.g. ON OVERFLOW ERROR Error, + + /// e.g. TRUNCATE [ ] WITH[OUT] COUNT Truncate { filler: Option>, with_count: bool, From d1ff8258fab793cf03fef291095865d003d49e5a Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 19:10:39 -0700 Subject: [PATCH 17/29] add back in implementation note --- src/parser.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/parser.rs b/src/parser.rs index 04c429348..f1162fd18 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -432,6 +432,9 @@ impl Parser { Some(ListAggOnOverflow::Error) } else { self.expect_keyword("TRUNCATE")?; + // While ANSI SQL would would require the separator, Redshift makes this optional. + // Here we choose to make the separator optional as this provides the more general + // implementation. let filler = match self.peek_token() { Some(tok) => { if tok == Token::make_keyword("WITH") From 87ac2a1aa0ae39dfcb31f826e6305a09d089e33e Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 19:11:21 -0700 Subject: [PATCH 18/29] relocate comment to proper place --- src/parser.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/parser.rs b/src/parser.rs index f1162fd18..311d4e143 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -422,6 +422,9 @@ impl Parser { self.expect_token(&Token::LParen)?; let distinct = self.parse_all_or_distinct()?; let expr = Box::new(self.parse_expr()?); + // While ANSI SQL would would require the separator, Redshift makes this optional. + // Here we choose to make the separator optional as this provides the more general + // implementation. let separator = if self.consume_token(&Token::Comma) { Some(Box::new(self.parse_expr()?)) } else { @@ -432,9 +435,6 @@ impl Parser { Some(ListAggOnOverflow::Error) } else { self.expect_keyword("TRUNCATE")?; - // While ANSI SQL would would require the separator, Redshift makes this optional. - // Here we choose to make the separator optional as this provides the more general - // implementation. let filler = match self.peek_token() { Some(tok) => { if tok == Token::make_keyword("WITH") From 95c5312606f84df3e77fa11012bafa2daa05eeb6 Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Fri, 29 May 2020 19:13:49 -0700 Subject: [PATCH 19/29] ensure proper comment formatting --- src/parser.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/parser.rs b/src/parser.rs index 311d4e143..15b9918f8 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -422,9 +422,8 @@ impl Parser { self.expect_token(&Token::LParen)?; let distinct = self.parse_all_or_distinct()?; let expr = Box::new(self.parse_expr()?); - // While ANSI SQL would would require the separator, Redshift makes this optional. - // Here we choose to make the separator optional as this provides the more general - // implementation. + // While ANSI SQL would would require the separator, Redshift makes this optional. Here we + // choose to make the separator optional as this provides the more general implementation. let separator = if self.consume_token(&Token::Comma) { Some(Box::new(self.parse_expr()?)) } else { From 4e17b475f2d7ae37402b2106a78fd1c6b3957f2b Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Sat, 30 May 2020 06:28:45 -0700 Subject: [PATCH 20/29] ensure backticks for formatting --- src/ast/mod.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/ast/mod.rs b/src/ast/mod.rs index 9d96549f9..ebdbfa22f 100644 --- a/src/ast/mod.rs +++ b/src/ast/mod.rs @@ -853,9 +853,8 @@ impl FromStr for FileFormat { } } -/// A `LISTAGG` invocation, e.g. -/// LISTAGG( [ DISTINCT ] [, ] [ON OVERFLOW ] ) -/// [ WITHIN GROUP (ORDER BY [, ...] ) ] +/// A `LISTAGG` invocation `LISTAGG( [ DISTINCT ] [, ] [ON OVERFLOW ] ) ) +/// [ WITHIN GROUP (ORDER BY [, ...] ) ]` #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct ListAgg { pub distinct: bool, @@ -894,13 +893,13 @@ impl fmt::Display for ListAgg { } } -/// The `ON OVERFLOW` clause of a LISTAGG invocation e.g. +/// The `ON OVERFLOW` clause of a LISTAGG invocation #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum ListAggOnOverflow { - /// e.g. ON OVERFLOW ERROR + /// `ON OVERFLOW ERROR` Error, - /// e.g. TRUNCATE [ ] WITH[OUT] COUNT + /// `ON OVERFLOW TRUNCATE [ ] WITH[OUT] COUNT` Truncate { filler: Option>, with_count: bool, @@ -1042,3 +1041,4 @@ impl fmt::Display for SetVariableValue { } } } +} From 7a258371ef8002ab4621ee6be8d6b127b12e4aa6 Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Sat, 30 May 2020 06:30:01 -0700 Subject: [PATCH 21/29] cleanup doc-comment --- src/ast/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ast/mod.rs b/src/ast/mod.rs index ebdbfa22f..5b63799bd 100644 --- a/src/ast/mod.rs +++ b/src/ast/mod.rs @@ -224,7 +224,7 @@ pub enum Expr { /// A parenthesized subquery `(SELECT ...)`, used in expression like /// `SELECT (subquery) AS x` or `WHERE (subquery) = x` Subquery(Box), - /// The `lISTAGG` function, e.g. `SELECT LISTAGG(...) WITHIN GROUP (ORDER BY ...)`. + /// The `LISTAGG` function `SELECT LISTAGG(...) WITHIN GROUP (ORDER BY ...)` ListAgg(ListAgg), } @@ -1041,4 +1041,3 @@ impl fmt::Display for SetVariableValue { } } } -} From 0596074eaf11cceabcd2f3eb14d3fea0d0e3a447 Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Sat, 30 May 2020 06:34:34 -0700 Subject: [PATCH 22/29] ensure consistent fmt implementation --- src/ast/mod.rs | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/src/ast/mod.rs b/src/ast/mod.rs index 5b63799bd..2dbf42b29 100644 --- a/src/ast/mod.rs +++ b/src/ast/mod.rs @@ -868,28 +868,25 @@ impl fmt::Display for ListAgg { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!( f, - "LISTAGG({}{}{})", + "LISTAGG({}{}", if self.distinct { "DISTINCT " } else { "" }, - if let Some(separator) = &self.separator { - format!("{}, {}", self.expr, separator) - } else { - format!("{}", self.expr) - }, - if let Some(on_overflow) = &self.on_overflow { - format!("{}", on_overflow) - } else { - "".to_string() - } + self.expr )?; + if let Some(separator) = &self.separator { + write!(f, ", {}", separator)?; + } + if let Some(on_overflow) = &self.on_overflow { + write!(f, "{}", on_overflow)?; + } + write!(f, ")")?; if !self.within_group.is_empty() { write!( f, " WITHIN GROUP (ORDER BY {})", display_comma_separated(&self.within_group) - ) - } else { - Ok(()) + )?; } + Ok(()) } } From f1f8c4a727b0685b54e118623756358651d5af2e Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Sat, 30 May 2020 07:06:51 -0700 Subject: [PATCH 23/29] clean up filler parsing a bit --- src/parser.rs | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/parser.rs b/src/parser.rs index 15b9918f8..86dd2a65a 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -435,21 +435,15 @@ impl Parser { } else { self.expect_keyword("TRUNCATE")?; let filler = match self.peek_token() { - Some(tok) => { - if tok == Token::make_keyword("WITH") - || tok == Token::make_keyword("WITHOUT") - { - None - } else { - Some(Box::new(self.parse_expr()?)) - } + Some(Token::Word(kw)) if kw.keyword == "WITH" || kw.keyword == "WITHOUT" => { + None } - None => None, + Some(Token::SingleQuotedString(_)) => Some(Box::new(self.parse_expr()?)), + _ => self.expected("either filler, WITH, or WITHOUT", self.peek_token())?, }; - let with_count = self.parse_keyword("WITH"); - if !with_count && !self.parse_keyword("WITHOUT") { - self.expected("either WITH or WITHOUT in LISTAGG", self.peek_token())?; + if !with_count { + self.expect_keyword("WITHOUT")?; } self.expect_keyword("COUNT")?; Some(ListAggOnOverflow::Truncate { filler, with_count }) From 5964599ad1ecb7ab78750449677fc06c9d3e3856 Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Sat, 30 May 2020 07:09:13 -0700 Subject: [PATCH 24/29] clarify error context --- src/parser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/parser.rs b/src/parser.rs index 86dd2a65a..58d8e2255 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -439,7 +439,7 @@ impl Parser { None } Some(Token::SingleQuotedString(_)) => Some(Box::new(self.parse_expr()?)), - _ => self.expected("either filler, WITH, or WITHOUT", self.peek_token())?, + _ => self.expected("either filler, WITH, or WITHOUT in LISTAGG", self.peek_token())?, }; let with_count = self.parse_keyword("WITH"); if !with_count { From ebe00d7f0b3ea1b29d92d41adf825ade8b28b802 Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Sat, 30 May 2020 07:38:27 -0700 Subject: [PATCH 25/29] expand valid filler types --- src/parser.rs | 9 +++++++-- tests/sqlparser_common.rs | 2 ++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/parser.rs b/src/parser.rs index 58d8e2255..c53d6cbeb 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -438,8 +438,13 @@ impl Parser { Some(Token::Word(kw)) if kw.keyword == "WITH" || kw.keyword == "WITHOUT" => { None } - Some(Token::SingleQuotedString(_)) => Some(Box::new(self.parse_expr()?)), - _ => self.expected("either filler, WITH, or WITHOUT in LISTAGG", self.peek_token())?, + Some(Token::SingleQuotedString(_)) + | Some(Token::NationalStringLiteral(_)) + | Some(Token::HexStringLiteral(_)) => Some(Box::new(self.parse_expr()?)), + _ => self.expected( + "either filler, WITH, or WITHOUT in LISTAGG", + self.peek_token(), + )?, }; let with_count = self.parse_keyword("WITH"); if !with_count { diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 9ff9bd769..be1ccf185 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -922,6 +922,8 @@ fn parse_listagg() { verified_stmt("SELECT LISTAGG(dateid)"); verified_stmt("SELECT LISTAGG(DISTINCT dateid)"); verified_stmt("SELECT LISTAGG(dateid ON OVERFLOW ERROR)"); + verified_stmt("SELECT LISTAGG(dateid ON OVERFLOW TRUNCATE N'...' WITH COUNT)"); + verified_stmt("SELECT LISTAGG(dateid ON OVERFLOW TRUNCATE X'deadbeef' WITH COUNT)"); let expr = Box::new(Expr::Identifier(Ident::new("dateid"))); let on_overflow = Some(ListAggOnOverflow::Truncate { From 815854f21333aa0ae041be4377069cbef2d338f9 Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Sat, 30 May 2020 08:07:18 -0700 Subject: [PATCH 26/29] revert back to with and without checking --- src/parser.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/parser.rs b/src/parser.rs index c53d6cbeb..c0345736f 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -447,8 +447,8 @@ impl Parser { )?, }; let with_count = self.parse_keyword("WITH"); - if !with_count { - self.expect_keyword("WITHOUT")?; + if !with_count && !self.parse_keyword("WITHOUT") { + self.expected("either WITH or WITHOUT in LISTAGG", self.peek_token())?; } self.expect_keyword("COUNT")?; Some(ListAggOnOverflow::Truncate { filler, with_count }) From 2b7d1167d65c439b980cb7c1fa244fd0216d6786 Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Sat, 30 May 2020 08:25:56 -0700 Subject: [PATCH 27/29] ensure nulls_first is provided --- tests/sqlparser_common.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index be1ccf185..0f1e97510 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -939,6 +939,7 @@ fn parse_listagg() { quote_style: None, }), asc: None, + nulls_first: None, }, OrderByExpr { expr: Expr::Identifier(Ident { @@ -946,6 +947,7 @@ fn parse_listagg() { quote_style: None, }), asc: None, + nulls_first: None }, ]; assert_eq!( From f92443bf47a184c1f8c3b4519189c73e749c6b5e Mon Sep 17 00:00:00 2001 From: Max Countryman Date: Sat, 30 May 2020 08:36:57 -0700 Subject: [PATCH 28/29] placate cargo fmt --- tests/sqlparser_common.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 0f1e97510..257b48230 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -947,7 +947,7 @@ fn parse_listagg() { quote_style: None, }), asc: None, - nulls_first: None + nulls_first: None, }, ]; assert_eq!( From cd471e8dd5395987cd035ff2ce0f1ae9c726a23f Mon Sep 17 00:00:00 2001 From: Nickolay Ponomarev Date: Sat, 30 May 2020 18:48:15 +0300 Subject: [PATCH 29/29] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d39c76cdb..60c25da18 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ Check https://github.com/andygrove/sqlparser-rs/commits/master for undocumented - Support `ON { UPDATE | DELETE } { RESTRICT | CASCADE | SET NULL | NO ACTION | SET DEFAULT }` in `FOREIGN KEY` constraints (#170) - thanks @c7hm4r! - Support basic forms of `CREATE SCHEMA` and `DROP SCHEMA` (#173) - thanks @alex-dukhno! - Support `NULLS FIRST`/`LAST` in `ORDER BY` expressions (#176) - thanks @houqp! +- Support `LISTAGG()` (#174) - thanks @maxcountryman! ### Fixed - Report an error for unterminated string literals (#165)