Support `GROUP BY WITH MODIFIER` for ClickHouse by git-hulk · Pull Request #1323 · apache/datafusion-sqlparser-rs

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

Jesse-Bakker mentioned this pull request Jul 29, 2024

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

Closed

Conversation

git-hulk commented Jun 25, 2024

Uh oh!

coveralls commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 9663460728

Details

💛 - Coveralls

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

git-hulk commented Jun 28, 2024

Uh oh!

coveralls commented Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 9708636908

Details

💛 - Coveralls

Uh oh!

coveralls commented Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 9708654389

Details

💛 - Coveralls

Uh oh!

iffyio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iffyio Jun 28, 2024

Choose a reason for hiding this comment

Uh oh!

git-hulk Jun 29, 2024

Choose a reason for hiding this comment

Uh oh!

iffyio Jun 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Jun 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 9720586492

Details

💛 - Coveralls

Uh oh!

coveralls commented Jun 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 9720588280

Details

💛 - Coveralls

Uh oh!

git-hulk commented Jun 29, 2024

Uh oh!

coveralls commented Jun 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 9720763225

Details

💛 - Coveralls

Uh oh!

iffyio left a comment

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Jun 30, 2024

Choose a reason for hiding this comment

Uh oh!

git-hulk Jun 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

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