Grouping sets dev by taylor12805 · Pull Request #26869 · ClickHouse/ClickHouse

taylor12805 · 2021-07-28T07:15:49Z

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
add grouping sets function, like GROUP BY grouping sets (a, b, (a, b))

Detailed description / Documentation draft:
Fix multiple grouping sets element issue from PR: #24172

taylor12805 · 2021-07-28T07:17:18Z

@KochetovNicolai Hi, please take a look at my implementation if you are free

taylor12805 · 2021-08-06T07:27:18Z

I find this implementation has performance issue because in AggregatingStep pipeline is resize to 1 and fork to number of grouping sets, which drastically decrease the performance

taylor12805 · 2021-08-23T06:34:05Z

@KochetovNicolai Hi, I come up with another implementation in Query Execution phase, using grouping sets transformer. The performance of this implementation is much higher than the one in #24172, please take a look to check if it is ok.

KochetovNicolai · 2021-08-26T08:40:33Z

@taylor12805 thank you! Will try to check it soon.

src/Interpreters/ExpressionAnalyzer.cpp

src/Core/ColumnNumbers.h

src/Core/NamesAndTypes.h

src/Interpreters/ExpressionAnalyzer.cpp

src/Interpreters/InterpreterSelectQuery.cpp

src/Processors/Pipe.cpp

src/Processors/QueryPlan/AggregatingStep.cpp

taylor12805 · 2021-09-06T07:48:14Z

There is bug when query on Distributed Table with grouping sets function, will try to fix it soon

novikd · 2021-09-06T09:07:30Z

src/Interpreters/InterpreterSelectQuery.cpp

Seems like you don't need this variable anymore.

src/Interpreters/InterpreterSelectQuery.cpp

taylor12805 · 2021-09-13T08:29:46Z

@novikd Hi, I generate reference file 01883_with_grouping_sets.reference following doc: https://clickhouse.tech/docs/en/development/adding_test_queries/, I have no idea why the test fails, please give me a hint

CLAassistant · 2021-09-28T10:42:45Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ novikd
❌ MaxTheHuman
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

…nsformer

2. Update formatAST to support grouping sets query with distributed table 3. modify astExpression to support function in grouping sets

azat · 2021-12-17T19:55:58Z

src/QueryPipeline/Pipe.h

    /// It is needed for debug. See QueryPipelineProcessorsCollector.
    Processors * collected_processors = nullptr;

+    Poco::Logger * log = &Poco::Logger::get("Pipe");


Looks unused.

azat · 2021-12-17T19:56:44Z

src/Processors/QueryPlan/AggregatingStep.h


    Processors aggregating;

+    Poco::Logger * log = &Poco::Logger::get("AggregatingStep");


This one also unused.

azat · 2021-12-17T19:57:13Z

src/Interpreters/ExpressionAnalyzer.h

        explicit ExtractedSettings(const Settings & settings_);
    };

+    Poco::Logger * poco_log = &Poco::Logger::get("ExpressionAnalyzer");


azat · 2021-12-17T19:57:38Z

src/Processors/Transforms/GroupingSetsTransform.h

+
+    UInt64 keys_vector_idx = 0;
+
+    Poco::Logger * log = &Poco::Logger::get("GroupingSetsTransform");


And this one is also unused.

sevirov · 2021-12-17T20:10:53Z

Internal documentation ticket: DOCSUP-20353

azat · 2021-12-18T08:39:33Z

src/Parsers/ASTSelectQuery.cpp

+    if (group_by_with_grouping_sets)
+    {
+        frame.surround_each_list_element_with_parens = true;
+        s.ostr << (s.hilite ? hilite_keyword : "") << s.nl_or_ws << indent_str << (s.one_line ? "" : "    ") << "GROUPING SETS" << (s.hilite ? hilite_none : "");
+        s.ostr << " (";
+        s.one_line
+        ? groupBy()->formatImpl(s, state, frame)
+        : groupBy()->as<ASTExpressionList &>().formatImplMultiline(s, state, frame);
+        s.ostr << ")";
+        frame.surround_each_list_element_with_parens = false;
+    }


Formatting looks odd:

SELECT key1, key2, sum(value) FROM data GROUP BY GROUPING SETS ( ( key1), ( key2))

So looks like:

surround_each_list_element_with_parens should not be set

extra parenthesis is not required

So something like this:

if (group_by_with_grouping_sets) { s.ostr << (s.hilite ? hilite_keyword : "") << s.nl_or_ws << indent_str << (s.one_line ? "" : " ") << "GROUPING SETS" << (s.hilite ? hilite_none : ""); s.one_line ? groupBy()->formatImpl(s, state, frame) : groupBy()->as<ASTExpressionList &>().formatImplMultiline(s, state, frame); }

And actually even newline between GROUP BY and GROUPING SETS looks reduntant.

extra parenthesis is not required

Why? This is part of GROUPING SETS syntax.

But I agree I should remove new line before GROUPING SETS and redundant whitespace before the first element in grouping set.

Why? This is part of GROUPING SETS syntax.

I don't see why they are required, both queries works fine:

SELECT key1, key2, sum(value) FROM data GROUP BY GROUPING SETS (( key1), ( key2))

SELECT key1, key2, sum(value) FROM data GROUP BY GROUPING SETS key1, key2

Plus GROUP BY keys are not surrounded with extra parenthesis.

Because grouping sets support several grouping commands.
This is a valid syntax:

GROUPING SETS ((key1), (key2), (key1, key2), ())

It should work similar to Postgres.

Got it, thanks for clarifications.

robot-clickhouse added doc-alert pr-feature Pull request with new product feature labels Jul 28, 2021

alexey-milovidov added the can be tested label Jul 28, 2021

alexey-milovidov requested a review from KochetovNicolai July 28, 2021 22:22

novikd self-assigned this Aug 29, 2021

novikd requested changes Sep 3, 2021

View reviewed changes

taylor12805 requested a review from novikd September 5, 2021 06:38

novikd reviewed Sep 6, 2021

View reviewed changes

src/Interpreters/InterpreterSelectQuery.cpp Outdated

Copy link
Copy Markdown

Member

novikd Sep 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you don't need this variable anymore.

novikd reviewed Sep 6, 2021

View reviewed changes

src/Interpreters/InterpreterSelectQuery.cpp Outdated Show resolved Hide resolved

taylor12805 requested a review from novikd September 13, 2021 08:39

This was referenced Nov 16, 2021

group sets #8330

Closed

TPC-DS compatibility issues #9887

Closed

MaxTheHuman added 11 commits November 23, 2021 17:58

init commit with parsing and BAD realisation

8b95b7d

rm unnessesary data committed by mistake

2403b03

remove another unnessesary files

4510911

revert changes made to cube transform

b693652

fix typo

66e74a8

erase blank line to restore initial state

b5bca80

fixes

391a601

fix typos

ff0bc94

feat grouping-sets: initial changes

2bd07ef

development

3418de3

grouping sets development

e60d1dd

fanzhou added 9 commits November 29, 2021 18:32

grouping sets support multiple group

2912344

grouing sets transformer instead of fork processor in aggregating tra…

534160b

…nsformer

fix bug when no data in grouping sets transformer

07a986f

correct test case results

ac6f08e

some changes

43db459

refactor

e725630

1. refactor ExpressionAnalyzer

b94f887

2. Update formatAST to support grouping sets query with distributed table 3. modify astExpression to support function in grouping sets

update test reference

5526c1c

update test reference

3bcf915

novikd force-pushed the grouping-sets-dev branch from b412dab to 3bcf915 Compare November 30, 2021 12:04

novikd added the force tests label Nov 30, 2021

novikd added 2 commits December 7, 2021 16:44

fix GROUPING SETS formating

f138122

remove ya.make

4afaa86

alexey-milovidov removed the can be tested label Dec 9, 2021

novikd added 2 commits December 14, 2021 22:15

Cleanup code

56a3f4a

Remove unused code

e8a96b5

novikd approved these changes Dec 14, 2021

View reviewed changes

Fix removing constant columns

0f3e2bb

alexey-milovidov merged commit 6b97af4 into ClickHouse:master Dec 17, 2021

novikd mentioned this pull request Dec 17, 2021

Add operator GROUPING SETS #24172

Closed

azat reviewed Dec 17, 2021

View reviewed changes

azat reviewed Dec 18, 2021

View reviewed changes

taylor12805 deleted the grouping-sets-dev branch December 21, 2021 13:39

alexey-milovidov mentioned this pull request Dec 25, 2021

Revert "Grouping sets dev" #33186

Merged

AngelLittleChaochao mentioned this pull request Jan 21, 2022

Can 'WITＨ ROLLUP' support a parameter? #33822

Closed

UnamedRus mentioned this pull request Apr 1, 2022

why grouping set is not supported ? #35828

Closed


		Processors aggregating;

		Poco::Logger * log = &Poco::Logger::get("AggregatingStep");


		UInt64 keys_vector_idx = 0;

		Poco::Logger * log = &Poco::Logger::get("GroupingSetsTransform");

Conversation

taylor12805 commented Jul 28, 2021

Uh oh!

taylor12805 commented Jul 28, 2021

Uh oh!

taylor12805 commented Aug 6, 2021

Uh oh!

taylor12805 commented Aug 23, 2021

Uh oh!

KochetovNicolai commented Aug 26, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taylor12805 commented Sep 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

taylor12805 commented Sep 13, 2021

Uh oh!

CLAassistant commented Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sevirov commented Dec 17, 2021

Uh oh!

azat Dec 18, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azat Dec 20, 2021 • 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

Labels

Projects

Milestone

Development

Uh oh!

9 participants

CLAassistant commented Sep 28, 2021 •

edited

Loading

azat Dec 18, 2021 •

edited

Loading

azat Dec 20, 2021 •

edited

Loading