Support `GROUP BY WITH MODIFIER` for ClickHouse #1323

git-hulk · 2024-06-25T13:32:30Z

For the syntax, please refer to:
https://clickhouse.com/docs/en/sql-reference/statements/select/group-by#rollup-modifier

For the syntax, please refer to: https://clickhouse.com/docs/en/sql-reference/statements/select/group-by#rollup-modifier

coveralls · 2024-06-25T13:36:31Z

Pull Request Test Coverage Report for Build 9663460728

Details

94 of 112 (83.93%) changed or added relevant lines in 8 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.03%) to 88.974%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser/mod.rs	14	16	87.5%
tests/sqlparser_common.rs	24	27	88.89%
tests/sqlparser_postgres.rs	25	28	89.29%
src/ast/query.rs	10	20	50.0%

Files with Coverage Reduction	New Missed Lines	%
src/parser/mod.rs	1	93.18%

Totals
Change from base Build 9633122504:	-0.03%
Covered Lines:	26314
Relevant Lines:	29575

💛 - Coveralls

src/ast/query.rs

tests/sqlparser_clickhouse.rs

src/parser/mod.rs

git-hulk · 2024-06-28T06:45:18Z

@iffyio Thanks for your great review and improvements. All of them are resolved, pls retake a look.

coveralls · 2024-06-28T06:48:20Z

Pull Request Test Coverage Report for Build 9708636908

Details

117 of 124 (94.35%) changed or added relevant lines in 8 files are covered.
153 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.06%) to 89.1%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser/mod.rs	12	13	92.31%
tests/sqlparser_common.rs	24	27	88.89%
tests/sqlparser_postgres.rs	25	28	89.29%

Files with Coverage Reduction	New Missed Lines	%
src/parser/mod.rs	1	93.21%
src/lib.rs	152	13.01%

Totals
Change from base Build 9696109634:	0.06%
Covered Lines:	26444
Relevant Lines:	29679

💛 - Coveralls

coveralls · 2024-06-28T06:50:24Z

Pull Request Test Coverage Report for Build 9708654389

Details

117 of 124 (94.35%) changed or added relevant lines in 8 files are covered.
153 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.06%) to 89.1%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser/mod.rs	12	13	92.31%
tests/sqlparser_common.rs	24	27	88.89%
tests/sqlparser_postgres.rs	25	28	89.29%

Files with Coverage Reduction	New Missed Lines	%
src/parser/mod.rs	1	93.21%
src/lib.rs	152	13.01%

Totals
Change from base Build 9696109634:	0.06%
Covered Lines:	26444
Relevant Lines:	29679

💛 - Coveralls

iffyio

@git-hulk left some minor comments, the PR looks good overall thanks!

src/ast/query.rs

iffyio · 2024-06-28T16:20:03Z

src/ast/query.rs

+                if col_names.is_empty() {
+                    return Ok(());
+                }


would it make sense to skip this check instead? if I'm understanding correctly, it seems to change the current behavior - if the list of expressions is empty, the current behavior looks like it will display GROUP BY, but this new behavior won't display anything. Not sure if we can end up with an empty list today but I imagine if we suceeded in parsing an empty list then we would want the display to include the GROUP BY that it parsed so the current behavior seems reasonable in any case?

@iffyio The old behavior also checks if the expressions is empty in Select's display: https://github.com/sqlparser-rs/sqlparser-rs/blob/main/src/ast/query.rs#L304. So I think GroupBy should be omitted if the expressions is empty and move the check to GroupBy itself.

Ah got it, thanks for clarifying! Yeah give that was the previous select behavior omits it then makes sense to preserve it

src/parser/mod.rs

Co-authored-by: Ifeanyi Ubah <[email protected]>

coveralls · 2024-06-29T01:45:32Z

Pull Request Test Coverage Report for Build 9720586492

Details

117 of 124 (94.35%) changed or added relevant lines in 8 files are covered.
153 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.06%) to 89.1%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser/mod.rs	12	13	92.31%
tests/sqlparser_common.rs	24	27	88.89%
tests/sqlparser_postgres.rs	25	28	89.29%

Files with Coverage Reduction	New Missed Lines	%
src/parser/mod.rs	1	93.21%
src/lib.rs	152	13.01%

Totals
Change from base Build 9696109634:	0.06%
Covered Lines:	26444
Relevant Lines:	29679

💛 - Coveralls

coveralls · 2024-06-29T01:45:53Z

Pull Request Test Coverage Report for Build 9720588280

Details

117 of 124 (94.35%) changed or added relevant lines in 8 files are covered.
153 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.06%) to 89.1%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser/mod.rs	12	13	92.31%
tests/sqlparser_common.rs	24	27	88.89%
tests/sqlparser_postgres.rs	25	28	89.29%

Files with Coverage Reduction	New Missed Lines	%
src/parser/mod.rs	1	93.21%
src/lib.rs	152	13.01%

Totals
Change from base Build 9696109634:	0.06%
Covered Lines:	26444
Relevant Lines:	29679

💛 - Coveralls

git-hulk · 2024-06-29T02:09:33Z

@iffyio Thanks for your review, the left comments are resolved. Please take a review while you get time.

coveralls · 2024-06-29T02:12:14Z

Pull Request Test Coverage Report for Build 9720763225

Details

118 of 125 (94.4%) changed or added relevant lines in 8 files are covered.
153 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.06%) to 89.101%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser/mod.rs	14	15	93.33%
tests/sqlparser_common.rs	24	27	88.89%
tests/sqlparser_postgres.rs	25	28	89.29%

Files with Coverage Reduction	New Missed Lines	%
src/parser/mod.rs	1	93.21%
src/lib.rs	152	13.01%

Totals
Change from base Build 9696109634:	0.06%
Covered Lines:	26446
Relevant Lines:	29681

💛 - Coveralls

iffyio

LGTM! cc @alamb

alamb

Thank you @git-hulk and @iffyio

alamb · 2024-06-30T11:33:08Z

tests/sqlparser_clickhouse.rs

+    }
+
+    // invalid cases
+    let invalid_cases = [


💯 for negative cases

This should be credited to the guidance of @iffyio, thank you!

git-hulk added 2 commits June 25, 2024 20:08

Add the ClickHouse reference to GroupBy All syntax

d22b5a2

Add support of GROUB BY WITH MODIFIER for ClickChouse

e3b641c

For the syntax, please refer to: https://clickhouse.com/docs/en/sql-reference/statements/select/group-by#rollup-modifier

iffyio reviewed Jun 28, 2024

View reviewed changes

Fix comment reviews

38867ab

git-hulk force-pushed the feature/group-by-with-modifiers branch from 121cb81 to 38867ab Compare June 28, 2024 06:46

git-hulk requested a review from iffyio June 28, 2024 06:55

iffyio reviewed Jun 28, 2024

View reviewed changes

git-hulk and others added 2 commits June 29, 2024 09:41

Update src/ast/query.rs

5470e58

Co-authored-by: Ifeanyi Ubah <[email protected]>

Update src/parser/mod.rs

42cfe5a

Co-authored-by: Ifeanyi Ubah <[email protected]>

Fix review comments

cc9d270

git-hulk requested a review from iffyio June 29, 2024 02:09

iffyio approved these changes Jun 29, 2024

View reviewed changes

alamb changed the title ~~Add support of GROUB BY WITH MODIFIER for ClickHouse~~ Support of GROUP BY WITH MODIFIER for ClickHouse Jun 30, 2024

alamb changed the title ~~Support of GROUP BY WITH MODIFIER for ClickHouse~~ Support GROUP BY WITH MODIFIER for ClickHouse Jun 30, 2024

alamb approved these changes Jun 30, 2024

View reviewed changes

alamb merged commit 44d7a20 into apache:main Jun 30, 2024
10 checks passed

Jesse-Bakker mentioned this pull request Jul 29, 2024

Parse ClickHouse table-valued function SETTINGS clause langdb/sqlparser-rs#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `GROUP BY WITH MODIFIER` for ClickHouse #1323

Support `GROUP BY WITH MODIFIER` for ClickHouse #1323

git-hulk commented Jun 25, 2024

coveralls commented Jun 25, 2024 •

edited

Loading

git-hulk commented Jun 28, 2024

coveralls commented Jun 28, 2024 •

edited

Loading

coveralls commented Jun 28, 2024 •

edited

Loading

iffyio left a comment

iffyio Jun 28, 2024

git-hulk Jun 29, 2024

iffyio Jun 29, 2024

coveralls commented Jun 29, 2024 •

edited

Loading

coveralls commented Jun 29, 2024 •

edited

Loading

git-hulk commented Jun 29, 2024

coveralls commented Jun 29, 2024 •

edited

Loading

iffyio left a comment

alamb left a comment

alamb Jun 30, 2024

git-hulk Jun 30, 2024 •

edited

Loading

Support GROUP BY WITH MODIFIER for ClickHouse #1323

Support GROUP BY WITH MODIFIER for ClickHouse #1323

Conversation

git-hulk commented Jun 25, 2024

coveralls commented Jun 25, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9663460728

Details

💛 - Coveralls

git-hulk commented Jun 28, 2024

coveralls commented Jun 28, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9708636908

Details

💛 - Coveralls

coveralls commented Jun 28, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9708654389

Details

💛 - Coveralls

iffyio left a comment

Choose a reason for hiding this comment

iffyio Jun 28, 2024

Choose a reason for hiding this comment

git-hulk Jun 29, 2024

Choose a reason for hiding this comment

iffyio Jun 29, 2024

Choose a reason for hiding this comment

coveralls commented Jun 29, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9720586492

Details

💛 - Coveralls

coveralls commented Jun 29, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9720588280

Details

💛 - Coveralls

git-hulk commented Jun 29, 2024

coveralls commented Jun 29, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9720763225

Details

💛 - Coveralls

iffyio left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Jun 30, 2024

Choose a reason for hiding this comment

git-hulk Jun 30, 2024 • edited Loading

Choose a reason for hiding this comment

Support `GROUP BY WITH MODIFIER` for ClickHouse #1323

Support `GROUP BY WITH MODIFIER` for ClickHouse #1323

coveralls commented Jun 25, 2024 •

edited

Loading

coveralls commented Jun 28, 2024 •

edited

Loading

coveralls commented Jun 28, 2024 •

edited

Loading

coveralls commented Jun 29, 2024 •

edited

Loading

coveralls commented Jun 29, 2024 •

edited

Loading

coveralls commented Jun 29, 2024 •

edited

Loading

git-hulk Jun 30, 2024 •

edited

Loading