Skip to content

Normalize count() variants#20175

Merged
vdimir merged 3 commits intoClickHouse:masterfrom
amosbird:countrewrite
Feb 11, 2021
Merged

Normalize count() variants#20175
vdimir merged 3 commits intoClickHouse:masterfrom
amosbird:countrewrite

Conversation

@amosbird
Copy link
Copy Markdown
Collaborator

@amosbird amosbird commented Feb 7, 2021

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

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Normalize count(constant), sum(1) to count(). This is needed for projection query routing.

Detailed description / Documentation draft:

.

@robot-clickhouse robot-clickhouse added the pr-improvement Pull request with some product improvements label Feb 7, 2021
vdimir
vdimir reviewed Feb 8, 2021
View reviewed changes
src/Core/Settings.h Outdated
Copy link
Copy Markdown
Member

@vdimir vdimir Feb 8, 2021

Choose a reason for hiding this comment

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

Why we don't want to perform transformations unconditionally?

Copy link
Copy Markdown
Collaborator Author

@amosbird amosbird Feb 8, 2021

Choose a reason for hiding this comment

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

good idea.

tests/queries/0_stateless/01706_optimize_normalize_count_variants.sql Outdated
Copy link
Copy Markdown
Member

@vdimir vdimir Feb 8, 2021

Choose a reason for hiding this comment

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

Let's add count(null) to ensure that it is not optimized.

Copy link
Copy Markdown
Collaborator Author

@amosbird amosbird Feb 8, 2021

Choose a reason for hiding this comment

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

Absolutely.

@vdimir vdimir self-assigned this Feb 8, 2021
@vdimir
Copy link
Copy Markdown
Member

vdimir commented Feb 8, 2021 •
edited
Loading

Build error:

2021-02-07 08:25:47 ../src/Interpreters/RewriteCountVariantsVisitor.h:13:17: error: function 'DB::RewriteCountVariantsVisitor::visit' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name,-warnings-as-errors]
2021-02-07 08:25:47     static void visit(ASTPtr & ast);
2021-02-07 08:25:47                 ^
2021-02-07 08:25:47 /build/obj-x86_64-linux-gnu/../src/Interpreters/RewriteCountVariantsVisitor.cpp:12:35: note: the definition seen here
2021-02-07 08:25:47 void RewriteCountVariantsVisitor::visit(ASTPtr & node)
2021-02-07 08:25:47                                   ^
2021-02-07 08:25:47 /build/obj-x86_64-linux-gnu/../src/Interpreters/RewriteCountVariantsVisitor.h:13:17: note: differing parameters are named here: ('ast'), in definition: ('node')
2021-02-07 08:25:47     static void visit(ASTPtr & ast);

https://clickhouse-builds.s3.yandex.net/20175/e14b8df2cf6da6f0aae5fb9821db9a84705ac0f8/clickhouse_special_build_check/report.html#fail1

@amosbird
Copy link
Copy Markdown
Collaborator Author

amosbird commented Feb 10, 2021

Test failure looks unrelated.

@vdimir vdimir merged commit 73a0e29 into ClickHouse:master Feb 11, 2021
@azat azat mentioned this pull request Feb 11, 2021
Merged
@abyss7
Copy link
Copy Markdown
Contributor

abyss7 commented Feb 11, 2021

Test failure looks unrelated.

It's related - it breaks master.

@alesapin alesapin mentioned this pull request Feb 11, 2021
Merged
@amosbird
Copy link
Copy Markdown
Collaborator Author

amosbird commented Feb 11, 2021

Test failure looks unrelated.

It's related - it breaks master.

Oops. The test failure looks quite obscure w.r.t to parser changes. I still don't see how it's related.

@abyss7
Copy link
Copy Markdown
Contributor

abyss7 commented Feb 15, 2021

Test failure looks unrelated.

It's related - it breaks master.

Oops. The test failure looks quite obscure w.r.t to parser changes. I still don't see how it's related.

With the new parser the IFunction::arguments may be nullptr

@taiyang-li taiyang-li mentioned this pull request Jan 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vdimir vdimir vdimir approved these changes

Assignees

@vdimir vdimir

Labels

pr-improvement Pull request with some product improvements

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@amosbird @vdimir @abyss7 @robot-clickhouse