From d22b5a24cdbb1817f25e24c4d1ef810e7e7f226c Mon Sep 17 00:00:00 2001 From: git-hulk Date: Tue, 25 Jun 2024 20:08:58 +0800 Subject: [PATCH 1/6] Add the ClickHouse reference to GroupBy All syntax --- src/ast/query.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ast/query.rs b/src/ast/query.rs index fcd5b970d..f7b4b2001 100644 --- a/src/ast/query.rs +++ b/src/ast/query.rs @@ -1869,10 +1869,11 @@ impl fmt::Display for SelectInto { #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] #[cfg_attr(feature = "visitor", derive(Visit, VisitMut))] pub enum GroupByExpr { - /// ALL syntax of [Snowflake], and [DuckDB] + /// ALL syntax of [Snowflake], [DuckDB] and [ClickHouse]. /// /// [Snowflake]: /// [DuckDB]: + /// [ClickHouse]: All, /// Expressions From e3b641c0528d55fff8899a73a345e21422a91628 Mon Sep 17 00:00:00 2001 From: git-hulk Date: Tue, 25 Jun 2024 21:30:08 +0800 Subject: [PATCH 2/6] Add support of GROUB BY WITH MODIFIER for ClickChouse For the syntax, please refer to: https://clickhouse.com/docs/en/sql-reference/statements/select/group-by#rollup-modifier --- src/ast/mod.rs | 2 +- src/ast/query.rs | 51 +++++++++++++++++++++++++----- src/keywords.rs | 1 + src/parser/mod.rs | 31 +++++++++++++++--- tests/sqlparser_clickhouse.rs | 20 +++++++++++- tests/sqlparser_common.rs | 53 +++++++++++++++++-------------- tests/sqlparser_duckdb.rs | 4 +-- tests/sqlparser_mssql.rs | 4 +-- tests/sqlparser_mysql.rs | 16 +++++----- tests/sqlparser_postgres.rs | 59 ++++++++++++++++++++--------------- 10 files changed, 168 insertions(+), 73 deletions(-) diff --git a/src/ast/mod.rs b/src/ast/mod.rs index 8182d1144..51001a22b 100644 --- a/src/ast/mod.rs +++ b/src/ast/mod.rs @@ -50,7 +50,7 @@ pub use self::query::{ RepetitionQuantifier, ReplaceSelectElement, ReplaceSelectItem, RowsPerMatch, Select, SelectInto, SelectItem, SetExpr, SetOperator, SetQuantifier, SymbolDefinition, Table, TableAlias, TableFactor, TableVersion, TableWithJoins, Top, TopQuantity, ValueTableMode, - Values, WildcardAdditionalOptions, With, + Values, WildcardAdditionalOptions, With, WithModifier, }; pub use self::value::{ escape_double_quote_string, escape_quoted_string, DateTimeField, DollarQuotedString, diff --git a/src/ast/query.rs b/src/ast/query.rs index f7b4b2001..255af7119 100644 --- a/src/ast/query.rs +++ b/src/ast/query.rs @@ -299,11 +299,19 @@ impl fmt::Display for Select { write!(f, " WHERE {selection}")?; } match &self.group_by { - GroupByExpr::All => write!(f, " GROUP BY ALL")?, - GroupByExpr::Expressions(exprs) => { + GroupByExpr::All(modifiers) => { + write!(f, " GROUP BY ALL")?; + if !modifiers.is_empty() { + write!(f, " {}", display_separated(modifiers, " "))?; + } + } + GroupByExpr::Expressions(exprs, modifiers) => { if !exprs.is_empty() { write!(f, " GROUP BY {}", display_comma_separated(exprs))?; } + if !modifiers.is_empty() { + write!(f, " {}", display_separated(modifiers, " "))?; + } } } if !self.cluster_by.is_empty() { @@ -1865,6 +1873,25 @@ impl fmt::Display for SelectInto { } } +#[derive(Debug, Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)] +#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] +#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))] +pub enum WithModifier { + Rollup, + Cube, + Totals, +} + +impl fmt::Display for WithModifier { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + WithModifier::Rollup => write!(f, "WITH ROLLUP"), + WithModifier::Cube => write!(f, "WITH CUBE"), + WithModifier::Totals => write!(f, "WITH TOTALS"), + } + } +} + #[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] #[cfg_attr(feature = "visitor", derive(Visit, VisitMut))] @@ -1874,19 +1901,29 @@ pub enum GroupByExpr { /// [Snowflake]: /// [DuckDB]: /// [ClickHouse]: - All, + /// + /// ClickHouse also supports WITH modifiers after GROUP BY ALL and expressions. + /// + /// [ClickHouse]: + All(Vec), /// Expressions - Expressions(Vec), + Expressions(Vec, Vec), } impl fmt::Display for GroupByExpr { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - GroupByExpr::All => write!(f, "GROUP BY ALL"), - GroupByExpr::Expressions(col_names) => { + GroupByExpr::All(modifiers) => { + write!(f, "GROUP BY ALL")?; + write!(f, " {}", display_separated(modifiers, " "))?; + Ok(()) + } + GroupByExpr::Expressions(col_names, modifiers) => { let col_names = display_comma_separated(col_names); - write!(f, "GROUP BY ({col_names})") + write!(f, "GROUP BY ({col_names})")?; + write!(f, " {}", display_separated(modifiers, " "))?; + Ok(()) } } } diff --git a/src/keywords.rs b/src/keywords.rs index e75d45e44..5db55e9da 100644 --- a/src/keywords.rs +++ b/src/keywords.rs @@ -721,6 +721,7 @@ define_keywords!( TINYINT, TO, TOP, + TOTALS, TRAILING, TRANSACTION, TRANSIENT, diff --git a/src/parser/mod.rs b/src/parser/mod.rs index 537609973..a09c4f7fa 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -8287,13 +8287,36 @@ impl<'a> Parser<'a> { }; let group_by = if self.parse_keywords(&[Keyword::GROUP, Keyword::BY]) { - if self.parse_keyword(Keyword::ALL) { - GroupByExpr::All + let expressions = if self.parse_keyword(Keyword::ALL) { + None } else { - GroupByExpr::Expressions(self.parse_comma_separated(Parser::parse_group_by_expr)?) + Some(self.parse_comma_separated(Parser::parse_group_by_expr)?) + }; + + let mut modifiers = vec![]; + loop { + if dialect_of!(self is ClickHouseDialect | GenericDialect) + && self.parse_keyword(Keyword::WITH) + { + if self.parse_keyword(Keyword::ROLLUP) { + modifiers.push(WithModifier::Rollup); + } else if self.parse_keyword(Keyword::CUBE) { + modifiers.push(WithModifier::Cube); + } else if self.parse_keyword(Keyword::TOTALS) { + modifiers.push(WithModifier::Totals); + } else { + self.expected("ROLLUP, CUBE, or TOTALS", self.peek_token())?; + } + } else { + break; + } + } + match expressions { + None => GroupByExpr::All(modifiers), + Some(exprs) => GroupByExpr::Expressions(exprs, modifiers), } } else { - GroupByExpr::Expressions(vec![]) + GroupByExpr::Expressions(vec![], vec![]) }; let cluster_by = if self.parse_keywords(&[Keyword::CLUSTER, Keyword::BY]) { diff --git a/tests/sqlparser_clickhouse.rs b/tests/sqlparser_clickhouse.rs index 50d4faf5d..418a01256 100644 --- a/tests/sqlparser_clickhouse.rs +++ b/tests/sqlparser_clickhouse.rs @@ -88,7 +88,7 @@ fn parse_map_access_expr() { right: Box::new(Expr::Value(Value::SingleQuotedString("foo".to_string()))), }), }), - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -626,6 +626,24 @@ fn parse_create_materialized_view() { clickhouse_and_generic().verified_stmt(sql); } +#[test] +fn parse_group_by_with_modifier() { + match clickhouse_and_generic().verified_stmt("SELECT * FROM t GROUP BY x WITH ROLLUP WITH CUBE") + { + Statement::Query(query) => { + let group_by = &query.body.as_select().unwrap().group_by; + assert_eq!( + group_by, + &GroupByExpr::Expressions( + vec![Identifier(Ident::new("x"))], + vec![WithModifier::Rollup, WithModifier::Cube] + ) + ); + } + _ => unreachable!(), + } +} + fn clickhouse() -> TestedDialects { TestedDialects { dialects: vec![Box::new(ClickHouseDialect {})], diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 76e6a98bb..ac2133946 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -392,9 +392,10 @@ fn parse_update_set_from() { }], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![Expr::Identifier(Ident::new( - "id" - ))]), + group_by: GroupByExpr::Expressions( + vec![Expr::Identifier(Ident::new("id"))], + vec![] + ), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -2119,10 +2120,13 @@ fn parse_select_group_by() { let sql = "SELECT id, fname, lname FROM customer GROUP BY lname, fname"; let select = verified_only_select(sql); assert_eq!( - GroupByExpr::Expressions(vec![ - Expr::Identifier(Ident::new("lname")), - Expr::Identifier(Ident::new("fname")), - ]), + GroupByExpr::Expressions( + vec![ + Expr::Identifier(Ident::new("lname")), + Expr::Identifier(Ident::new("fname")), + ], + vec![] + ), select.group_by ); @@ -2137,7 +2141,7 @@ fn parse_select_group_by() { fn parse_select_group_by_all() { let sql = "SELECT id, fname, lname, SUM(order) FROM customer GROUP BY ALL"; let select = verified_only_select(sql); - assert_eq!(GroupByExpr::All, select.group_by); + assert_eq!(GroupByExpr::All(vec![]), select.group_by); one_statement_parses_to( "SELECT id, fname, lname, SUM(order) FROM customer GROUP BY ALL", @@ -4545,7 +4549,7 @@ fn test_parse_named_window() { }], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -4974,7 +4978,7 @@ fn parse_interval_and_or_xor() { }), }), }), - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -6908,7 +6912,7 @@ fn lateral_function() { }], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -7627,7 +7631,7 @@ fn parse_merge() { }], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -9133,7 +9137,7 @@ fn parse_unload() { }], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -9276,7 +9280,7 @@ fn parse_connect_by() { into: None, lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -9364,7 +9368,7 @@ fn parse_connect_by() { op: BinaryOperator::NotEq, right: Box::new(Expr::Value(number("42"))), }), - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -9484,15 +9488,18 @@ fn test_group_by_grouping_sets() { all_dialects_where(|d| d.supports_group_by_expr()) .verified_only_select(sql) .group_by, - GroupByExpr::Expressions(vec![Expr::GroupingSets(vec![ - vec![ - Expr::Identifier(Ident::new("city")), - Expr::Identifier(Ident::new("car_model")) - ], - vec![Expr::Identifier(Ident::new("city")),], - vec![Expr::Identifier(Ident::new("car_model"))], + GroupByExpr::Expressions( + vec![Expr::GroupingSets(vec![ + vec![ + Expr::Identifier(Ident::new("city")), + Expr::Identifier(Ident::new("car_model")) + ], + vec![Expr::Identifier(Ident::new("city")),], + vec![Expr::Identifier(Ident::new("car_model"))], + vec![] + ])], vec![] - ])]) + ) ); } diff --git a/tests/sqlparser_duckdb.rs b/tests/sqlparser_duckdb.rs index eaa1faa90..0702014bc 100644 --- a/tests/sqlparser_duckdb.rs +++ b/tests/sqlparser_duckdb.rs @@ -171,7 +171,7 @@ fn test_select_union_by_name() { }], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -209,7 +209,7 @@ fn test_select_union_by_name() { }], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], diff --git a/tests/sqlparser_mssql.rs b/tests/sqlparser_mssql.rs index 5f03bb093..993850299 100644 --- a/tests/sqlparser_mssql.rs +++ b/tests/sqlparser_mssql.rs @@ -111,7 +111,7 @@ fn parse_create_procedure() { from: vec![], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -528,7 +528,7 @@ fn parse_substring_in_select() { }], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], diff --git a/tests/sqlparser_mysql.rs b/tests/sqlparser_mysql.rs index a25f4c208..4c18d4a75 100644 --- a/tests/sqlparser_mysql.rs +++ b/tests/sqlparser_mysql.rs @@ -907,7 +907,7 @@ fn parse_escaped_quote_identifiers_with_escape() { from: vec![], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -954,7 +954,7 @@ fn parse_escaped_quote_identifiers_with_no_escape() { from: vec![], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -998,7 +998,7 @@ fn parse_escaped_backticks_with_escape() { from: vec![], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -1042,7 +1042,7 @@ fn parse_escaped_backticks_with_no_escape() { from: vec![], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -1703,7 +1703,7 @@ fn parse_select_with_numeric_prefix_column_name() { }], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -1756,7 +1756,7 @@ fn parse_select_with_concatenation_of_exp_number_and_numeric_prefix_column() { }], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -2255,7 +2255,7 @@ fn parse_substring_in_select() { }], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -2559,7 +2559,7 @@ fn parse_hex_string_introducer() { from: vec![], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], diff --git a/tests/sqlparser_postgres.rs b/tests/sqlparser_postgres.rs index 197597e9b..2606fb96e 100644 --- a/tests/sqlparser_postgres.rs +++ b/tests/sqlparser_postgres.rs @@ -1075,7 +1075,7 @@ fn parse_copy_to() { from: vec![], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), having: None, named_window: vec![], window_before_qualify: false, @@ -2383,7 +2383,7 @@ fn parse_array_subquery_expr() { from: vec![], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -2402,7 +2402,7 @@ fn parse_array_subquery_expr() { from: vec![], lateral_views: vec![], selection: None, - group_by: GroupByExpr::Expressions(vec![]), + group_by: GroupByExpr::Expressions(vec![], vec![]), cluster_by: vec![], distribute_by: vec![], sort_by: vec![], @@ -3711,14 +3711,17 @@ fn parse_select_group_by_grouping_sets() { "SELECT brand, size, sum(sales) FROM items_sold GROUP BY size, GROUPING SETS ((brand), (size), ())" ); assert_eq!( - GroupByExpr::Expressions(vec![ - Expr::Identifier(Ident::new("size")), - Expr::GroupingSets(vec![ - vec![Expr::Identifier(Ident::new("brand"))], - vec![Expr::Identifier(Ident::new("size"))], - vec![], - ]), - ]), + GroupByExpr::Expressions( + vec![ + Expr::Identifier(Ident::new("size")), + Expr::GroupingSets(vec![ + vec![Expr::Identifier(Ident::new("brand"))], + vec![Expr::Identifier(Ident::new("size"))], + vec![], + ]), + ], + vec![] + ), select.group_by ); } @@ -3729,13 +3732,16 @@ fn parse_select_group_by_rollup() { "SELECT brand, size, sum(sales) FROM items_sold GROUP BY size, ROLLUP (brand, size)", ); assert_eq!( - GroupByExpr::Expressions(vec![ - Expr::Identifier(Ident::new("size")), - Expr::Rollup(vec![ - vec![Expr::Identifier(Ident::new("brand"))], - vec![Expr::Identifier(Ident::new("size"))], - ]), - ]), + GroupByExpr::Expressions( + vec![ + Expr::Identifier(Ident::new("size")), + Expr::Rollup(vec![ + vec![Expr::Identifier(Ident::new("brand"))], + vec![Expr::Identifier(Ident::new("size"))], + ]), + ], + vec![] + ), select.group_by ); } @@ -3746,13 +3752,16 @@ fn parse_select_group_by_cube() { "SELECT brand, size, sum(sales) FROM items_sold GROUP BY size, CUBE (brand, size)", ); assert_eq!( - GroupByExpr::Expressions(vec![ - Expr::Identifier(Ident::new("size")), - Expr::Cube(vec![ - vec![Expr::Identifier(Ident::new("brand"))], - vec![Expr::Identifier(Ident::new("size"))], - ]), - ]), + GroupByExpr::Expressions( + vec![ + Expr::Identifier(Ident::new("size")), + Expr::Cube(vec![ + vec![Expr::Identifier(Ident::new("brand"))], + vec![Expr::Identifier(Ident::new("size"))], + ]), + ], + vec![] + ), select.group_by ); } From 38867ab52dcbda1978b1be2b332db1d3f1d62a62 Mon Sep 17 00:00:00 2001 From: git-hulk Date: Fri, 28 Jun 2024 14:44:11 +0800 Subject: [PATCH 3/6] Fix comment reviews --- src/ast/mod.rs | 6 ++-- src/ast/query.rs | 50 +++++++++++++--------------- src/parser/mod.rs | 29 +++++++++-------- tests/sqlparser_clickhouse.rs | 61 ++++++++++++++++++++++++++++------- 4 files changed, 90 insertions(+), 56 deletions(-) diff --git a/src/ast/mod.rs b/src/ast/mod.rs index 51001a22b..9e92ff087 100644 --- a/src/ast/mod.rs +++ b/src/ast/mod.rs @@ -43,14 +43,14 @@ pub use self::operator::{BinaryOperator, UnaryOperator}; pub use self::query::{ AfterMatchSkip, ConnectBy, Cte, CteAsMaterialized, Distinct, EmptyMatchesMode, ExceptSelectItem, ExcludeSelectItem, ExprWithAlias, Fetch, ForClause, ForJson, ForXml, - GroupByExpr, IdentWithAlias, IlikeSelectItem, Join, JoinConstraint, JoinOperator, - JsonTableColumn, JsonTableColumnErrorHandling, LateralView, LockClause, LockType, + GroupByExpr, GroupByWithModifier, IdentWithAlias, IlikeSelectItem, Join, JoinConstraint, + JoinOperator, JsonTableColumn, JsonTableColumnErrorHandling, LateralView, LockClause, LockType, MatchRecognizePattern, MatchRecognizeSymbol, Measure, NamedWindowDefinition, NamedWindowExpr, NonBlock, Offset, OffsetRows, OrderByExpr, PivotValueSource, Query, RenameSelectItem, RepetitionQuantifier, ReplaceSelectElement, ReplaceSelectItem, RowsPerMatch, Select, SelectInto, SelectItem, SetExpr, SetOperator, SetQuantifier, SymbolDefinition, Table, TableAlias, TableFactor, TableVersion, TableWithJoins, Top, TopQuantity, ValueTableMode, - Values, WildcardAdditionalOptions, With, WithModifier, + Values, WildcardAdditionalOptions, With, }; pub use self::value::{ escape_double_quote_string, escape_quoted_string, DateTimeField, DollarQuotedString, diff --git a/src/ast/query.rs b/src/ast/query.rs index 255af7119..ad87f785c 100644 --- a/src/ast/query.rs +++ b/src/ast/query.rs @@ -298,22 +298,7 @@ impl fmt::Display for Select { if let Some(ref selection) = self.selection { write!(f, " WHERE {selection}")?; } - match &self.group_by { - GroupByExpr::All(modifiers) => { - write!(f, " GROUP BY ALL")?; - if !modifiers.is_empty() { - write!(f, " {}", display_separated(modifiers, " "))?; - } - } - GroupByExpr::Expressions(exprs, modifiers) => { - if !exprs.is_empty() { - write!(f, " GROUP BY {}", display_comma_separated(exprs))?; - } - if !modifiers.is_empty() { - write!(f, " {}", display_separated(modifiers, " "))?; - } - } - } + write!(f, "{}", self.group_by)?; if !self.cluster_by.is_empty() { write!( f, @@ -1876,18 +1861,22 @@ impl fmt::Display for SelectInto { #[derive(Debug, Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] #[cfg_attr(feature = "visitor", derive(Visit, VisitMut))] -pub enum WithModifier { +pub enum GroupByWithModifier { + /// ClickHouse supports GROUP BY WITH modifiers(includes ROLLUP|CUBE|TOTALS). + /// e.g. GROUP BY year WITH ROLLUP WITH TOTALS + /// + /// [ClickHouse]: Rollup, Cube, Totals, } -impl fmt::Display for WithModifier { +impl fmt::Display for GroupByWithModifier { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - WithModifier::Rollup => write!(f, "WITH ROLLUP"), - WithModifier::Cube => write!(f, "WITH CUBE"), - WithModifier::Totals => write!(f, "WITH TOTALS"), + GroupByWithModifier::Rollup => write!(f, "WITH ROLLUP"), + GroupByWithModifier::Cube => write!(f, "WITH CUBE"), + GroupByWithModifier::Totals => write!(f, "WITH TOTALS"), } } } @@ -1905,24 +1894,31 @@ pub enum GroupByExpr { /// ClickHouse also supports WITH modifiers after GROUP BY ALL and expressions. /// /// [ClickHouse]: - All(Vec), + All(Vec), /// Expressions - Expressions(Vec, Vec), + Expressions(Vec, Vec), } impl fmt::Display for GroupByExpr { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { GroupByExpr::All(modifiers) => { - write!(f, "GROUP BY ALL")?; - write!(f, " {}", display_separated(modifiers, " "))?; + write!(f, " GROUP BY ALL")?; + if !modifiers.is_empty() { + write!(f, " {}", display_separated(modifiers, " "))?; + } Ok(()) } GroupByExpr::Expressions(col_names, modifiers) => { + if col_names.is_empty() { + return Ok(()); + } let col_names = display_comma_separated(col_names); - write!(f, "GROUP BY ({col_names})")?; - write!(f, " {}", display_separated(modifiers, " "))?; + write!(f, " GROUP BY {col_names}")?; + if !modifiers.is_empty() { + write!(f, " {}", display_separated(modifiers, " "))?; + } Ok(()) } } diff --git a/src/parser/mod.rs b/src/parser/mod.rs index a09c4f7fa..d3da73ac9 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -8294,21 +8294,22 @@ impl<'a> Parser<'a> { }; let mut modifiers = vec![]; - loop { - if dialect_of!(self is ClickHouseDialect | GenericDialect) - && self.parse_keyword(Keyword::WITH) - { - if self.parse_keyword(Keyword::ROLLUP) { - modifiers.push(WithModifier::Rollup); - } else if self.parse_keyword(Keyword::CUBE) { - modifiers.push(WithModifier::Cube); - } else if self.parse_keyword(Keyword::TOTALS) { - modifiers.push(WithModifier::Totals); - } else { - self.expected("ROLLUP, CUBE, or TOTALS", self.peek_token())?; + if dialect_of!(self is ClickHouseDialect | GenericDialect) { + loop { + if !self.parse_keyword(Keyword::WITH) { + break; } - } else { - break; + let keyword = self.expect_one_of_keywords(&[ + Keyword::ROLLUP, + Keyword::CUBE, + Keyword::TOTALS, + ])?; + modifiers.push(match keyword { + Keyword::ROLLUP => GroupByWithModifier::Rollup, + Keyword::CUBE => GroupByWithModifier::Cube, + Keyword::TOTALS => GroupByWithModifier::Totals, + _ => unreachable!(), + }); } } match expressions { diff --git a/tests/sqlparser_clickhouse.rs b/tests/sqlparser_clickhouse.rs index 418a01256..0c188a24b 100644 --- a/tests/sqlparser_clickhouse.rs +++ b/tests/sqlparser_clickhouse.rs @@ -628,19 +628,56 @@ fn parse_create_materialized_view() { #[test] fn parse_group_by_with_modifier() { - match clickhouse_and_generic().verified_stmt("SELECT * FROM t GROUP BY x WITH ROLLUP WITH CUBE") - { - Statement::Query(query) => { - let group_by = &query.body.as_select().unwrap().group_by; - assert_eq!( - group_by, - &GroupByExpr::Expressions( - vec![Identifier(Ident::new("x"))], - vec![WithModifier::Rollup, WithModifier::Cube] - ) - ); + let clauses = ["x", "a, b", "ALL"]; + let modifiers = [ + "WITH ROLLUP", + "WITH CUBE", + "WITH TOTALS", + "WITH ROLLUP WITH CUBE", + ]; + let expected_modifiers = [ + vec![GroupByWithModifier::Rollup], + vec![GroupByWithModifier::Cube], + vec![GroupByWithModifier::Totals], + vec![GroupByWithModifier::Rollup, GroupByWithModifier::Cube], + ]; + for clause in &clauses { + for (modifier, expected_modifier) in modifiers.iter().zip(expected_modifiers.iter()) { + let sql = format!("SELECT * FROM t GROUP BY {clause} {modifier}"); + match clickhouse_and_generic().verified_stmt(&sql) { + Statement::Query(query) => { + let group_by = &query.body.as_select().unwrap().group_by; + if clause == &"ALL" { + assert_eq!(group_by, &GroupByExpr::All(expected_modifier.to_vec())); + } else { + assert_eq!( + group_by, + &GroupByExpr::Expressions( + clause + .split(", ") + .map(|c| Identifier(Ident::new(c))) + .collect(), + expected_modifier.to_vec() + ) + ); + } + } + _ => unreachable!(), + } } - _ => unreachable!(), + } + + // invalid cases + let invalid_cases = [ + "SELECT * FROM t GROUP BY x WITH", + "SELECT * FROM t GROUP BY x WITH ROLLUP CUBE", + "SELECT * FROM t GROUP BY x WITH WITH ROLLUP", + "SELECT * FROM t GROUP BY WITH ROLLUP", + ]; + for sql in invalid_cases { + clickhouse_and_generic() + .parse_sql_statements(sql) + .expect_err("Expected: one of ROLLUP or CUBE or TOTALS, found: WITH"); } } From 5470e5885dbf9aa6b9eaf365b9d99e792a642519 Mon Sep 17 00:00:00 2001 From: hulk Date: Sat, 29 Jun 2024 09:41:26 +0800 Subject: [PATCH 4/6] Update src/ast/query.rs Co-authored-by: Ifeanyi Ubah --- src/ast/query.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ast/query.rs b/src/ast/query.rs index ad87f785c..2bc41a60b 100644 --- a/src/ast/query.rs +++ b/src/ast/query.rs @@ -1858,14 +1858,14 @@ impl fmt::Display for SelectInto { } } +/// ClickHouse supports GROUP BY WITH modifiers(includes ROLLUP|CUBE|TOTALS). +/// e.g. GROUP BY year WITH ROLLUP WITH TOTALS +/// +/// [ClickHouse]: #[derive(Debug, Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] #[cfg_attr(feature = "visitor", derive(Visit, VisitMut))] pub enum GroupByWithModifier { - /// ClickHouse supports GROUP BY WITH modifiers(includes ROLLUP|CUBE|TOTALS). - /// e.g. GROUP BY year WITH ROLLUP WITH TOTALS - /// - /// [ClickHouse]: Rollup, Cube, Totals, From 42cfe5ae682769852bc1df9b0fe873ff5e055b1f Mon Sep 17 00:00:00 2001 From: hulk Date: Sat, 29 Jun 2024 09:41:40 +0800 Subject: [PATCH 5/6] Update src/parser/mod.rs Co-authored-by: Ifeanyi Ubah --- src/parser/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/parser/mod.rs b/src/parser/mod.rs index d3da73ac9..3bed73276 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -8308,7 +8308,7 @@ impl<'a> Parser<'a> { Keyword::ROLLUP => GroupByWithModifier::Rollup, Keyword::CUBE => GroupByWithModifier::Cube, Keyword::TOTALS => GroupByWithModifier::Totals, - _ => unreachable!(), + _ => return parser_err!("BUG: expected to match GroupBy modifier keyword", self.peek_token().location), }); } } From cc9d270704344bad4c35b3afbf5383cb15c0ada5 Mon Sep 17 00:00:00 2001 From: git-hulk Date: Sat, 29 Jun 2024 10:07:46 +0800 Subject: [PATCH 6/6] Fix review comments --- src/ast/query.rs | 16 ++++++++++------ src/parser/mod.rs | 7 ++++++- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/ast/query.rs b/src/ast/query.rs index 2bc41a60b..ba207f9ff 100644 --- a/src/ast/query.rs +++ b/src/ast/query.rs @@ -298,7 +298,14 @@ impl fmt::Display for Select { if let Some(ref selection) = self.selection { write!(f, " WHERE {selection}")?; } - write!(f, "{}", self.group_by)?; + match &self.group_by { + GroupByExpr::All(_) => write!(f, " {}", self.group_by)?, + GroupByExpr::Expressions(exprs, _) => { + if !exprs.is_empty() { + write!(f, " {}", self.group_by)? + } + } + } if !self.cluster_by.is_empty() { write!( f, @@ -1904,18 +1911,15 @@ impl fmt::Display for GroupByExpr { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { GroupByExpr::All(modifiers) => { - write!(f, " GROUP BY ALL")?; + write!(f, "GROUP BY ALL")?; if !modifiers.is_empty() { write!(f, " {}", display_separated(modifiers, " "))?; } Ok(()) } GroupByExpr::Expressions(col_names, modifiers) => { - if col_names.is_empty() { - return Ok(()); - } let col_names = display_comma_separated(col_names); - write!(f, " GROUP BY {col_names}")?; + write!(f, "GROUP BY {col_names}")?; if !modifiers.is_empty() { write!(f, " {}", display_separated(modifiers, " "))?; } diff --git a/src/parser/mod.rs b/src/parser/mod.rs index 3bed73276..842aa50de 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -8308,7 +8308,12 @@ impl<'a> Parser<'a> { Keyword::ROLLUP => GroupByWithModifier::Rollup, Keyword::CUBE => GroupByWithModifier::Cube, Keyword::TOTALS => GroupByWithModifier::Totals, - _ => return parser_err!("BUG: expected to match GroupBy modifier keyword", self.peek_token().location), + _ => { + return parser_err!( + "BUG: expected to match GroupBy modifier keyword", + self.peek_token().location + ) + } }); } }