Skip to content

Cleanup GROUPING SETS formating#32999

Merged
novikd merged 3 commits intomasterfrom
fix-gs-formating
May 15, 2022
Merged

Cleanup GROUPING SETS formating#32999
novikd merged 3 commits intomasterfrom
fix-gs-formating

Conversation

@novikd
Copy link
Copy Markdown
Member

@novikd novikd commented Dec 21, 2021

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

@robot-clickhouse robot-clickhouse added the pr-not-for-changelog This PR should not be mentioned in the changelog label Dec 21, 2021
azat
azat reviewed Dec 21, 2021
View reviewed changes
tests/queries/0_stateless/01883_with_grouping_sets.reference
GROUP BY
GROUPING SETS (
(fact_1_id),
(fact_1_id, fact_3_id))
Copy link
Copy Markdown
Member

@azat azat Dec 21, 2021

Choose a reason for hiding this comment

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

What do you think about improving the formatting with one element?

Right now (including this changes) it includes extra whitespace:

SELECT *
FROM data
GROUP BY
    GROUPING SETS ( (foo))

Either this:

SELECT *
FROM data
GROUP BY
    GROUPING SETS ((foo))

Or this:

SELECT *
FROM data
GROUP BY
    GROUPING SETS (
        (foo)
    )

Copy link
Copy Markdown
Member

@alexey-milovidov alexey-milovidov Dec 21, 2021

Choose a reason for hiding this comment

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

Thank you! I also cannot stand extra whitespace :)

Copy link
Copy Markdown
Member

@alexey-milovidov alexey-milovidov Dec 21, 2021

Choose a reason for hiding this comment

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

Second example is ok.

@novikd
Copy link
Copy Markdown
Member Author

novikd commented Dec 23, 2021

@Mergifyio update

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Dec 23, 2021

update

✅ Branch has been successfully updated

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Dec 24, 2021

Looks like there are some defects in GROUPING SETS implementation?

@alexey-milovidov alexey-milovidov marked this pull request as draft January 30, 2022 00:07
@novikd novikd mentioned this pull request Apr 29, 2022
Merged
@novikd
Copy link
Copy Markdown
Member Author

novikd commented May 11, 2022

@Mergifyio rebase

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented May 11, 2022

rebase

❌ Base branch update has failed

Details

Git reported the following error:

Rebasing (1/2)
warning: inexact rename detection was skipped due to too many files.
warning: you may want to set your merge.renamelimit variable to at least 1844 and retry the command.
error: could not apply 5047cdec00... Cleanup GROUPING SETS formating
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 5047cdec00... Cleanup GROUPING SETS formating
Auto-merging tests/queries/0_stateless/01883_with_grouping_sets.sql
Auto-merging tests/queries/0_stateless/01883_with_grouping_sets.reference
CONFLICT (content): Merge conflict in tests/queries/0_stateless/01883_with_grouping_sets.reference
Auto-merging src/QueryPipeline/Pipe.h
CONFLICT (modify/delete): src/Processors/Transforms/GroupingSetsTransform.h deleted in HEAD and modified in 5047cdec00 (Cleanup GROUPING SETS formating). Version 5047cdec00 (Cleanup GROUPING SETS formating) of src/Processors/Transforms/GroupingSetsTransform.h left in tree.
Auto-merging src/Processors/QueryPlan/AggregatingStep.h
Auto-merging src/Parsers/ASTSelectQuery.cpp
Auto-merging src/Interpreters/ExpressionAnalyzer.h

err-code: 33C22

@novikd novikd force-pushed the fix-gs-formating branch from 03bf4e7 to 5128afb Compare May 13, 2022 16:18
@novikd novikd marked this pull request as ready for review May 13, 2022 16:20
@novikd
Copy link
Copy Markdown
Member Author

novikd commented May 13, 2022

@Mergifyio update

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented May 13, 2022

update

☑️ Nothing to do

Details
  • -closed [:pushpin: update requirement]
  • #commits-behind>0 [:pushpin: update requirement]

@novikd novikd merged commit a36f05d into master May 15, 2022
@novikd novikd deleted the fix-gs-formating branch May 15, 2022 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@azat azat azat left review comments

@alexey-milovidov alexey-milovidov alexey-milovidov left review comments

Assignees

No one assigned

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@novikd @alexey-milovidov @azat @robot-clickhouse