Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a bug with projections and the aggregate_functions_null_for_empty setting #42198

Merged
merged 4 commits into from Oct 14, 2022

Conversation

alexey-milovidov
Copy link
Member

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in official stable or prestable release)

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

Fix a bug with projections and the aggregate_functions_null_for_empty setting. This bug is very rare and appears only if you enable the aggregate_functions_null_for_empty setting in the server's config. This closes #41647.

@alexey-milovidov alexey-milovidov added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Oct 9, 2022
@robot-ch-test-poll robot-ch-test-poll added the pr-bugfix Pull request with bugfix, not backported by default label Oct 9, 2022
@@ -176,6 +176,11 @@ QueryPlanPtr MergeTreeDataSelectExecutor::read(
"No projection is used when allow_experimental_projection_optimization = 1 and force_optimize_projection = 1",
ErrorCodes::PROJECTION_NOT_USED);

if (settings.force_optimize_projection && settings.aggregate_functions_null_for_empty)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This part looks useless.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why useless?

I treat it this way: if a user requested "force_optimize_projection" it should know if the optimization is not applied.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if a user requested "force_optimize_projection" it should know if the optimization is not applied.

The exception thrown a few lines above carries such meaning exactly.

If there is some projection define in the table, and for any reason (like aggregate_functions_null_for_empty = true) it cannot be used, the exception will be thrown.

The setting was to mimic the behavior of force_primary_key, which has the description: Throw an exception if there is primary key in a table, and it is not used.. However, I just did a quick test and found that it will throw even when table doesn't have primary key. Not sure whether we should change the description or the behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

However, I just did a quick test and found that it will throw even when table doesn't have primary key. Not sure whether we should change the description or the behavior.

Better to change the behavior. (Don't throw if no primary key)

@amosbird
Copy link
Collaborator

amosbird commented Oct 9, 2022

LGTM.

@SmitaRKulkarni SmitaRKulkarni self-assigned this Oct 11, 2022
Copy link
Member

@SmitaRKulkarni SmitaRKulkarni left a comment

Choose a reason for hiding this comment

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

LGTM

@alexey-milovidov alexey-milovidov merged commit 018c68b into master Oct 14, 2022
@alexey-milovidov alexey-milovidov deleted the fix-projections branch October 14, 2022 01:14
robot-clickhouse pushed a commit that referenced this pull request Oct 14, 2022
robot-clickhouse pushed a commit that referenced this pull request Oct 14, 2022
robot-clickhouse pushed a commit that referenced this pull request Oct 14, 2022
robot-clickhouse pushed a commit that referenced this pull request Oct 14, 2022
@robot-ch-test-poll robot-ch-test-poll added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Oct 14, 2022
alexey-milovidov added a commit that referenced this pull request Oct 14, 2022
Backport #42198 to 22.9: Fix a bug with projections and the `aggregate_functions_null_for_empty` setting
alexey-milovidov added a commit that referenced this pull request Oct 14, 2022
Backport #42198 to 22.8: Fix a bug with projections and the `aggregate_functions_null_for_empty` setting
alexey-milovidov added a commit that referenced this pull request Oct 14, 2022
Backport #42198 to 22.7: Fix a bug with projections and the `aggregate_functions_null_for_empty` setting
alexey-milovidov added a commit that referenced this pull request Oct 14, 2022
Backport #42198 to 22.3: Fix a bug with projections and the `aggregate_functions_null_for_empty` setting
ucasfl pushed a commit to ucasfl/ClickHouse that referenced this pull request Nov 30, 2022
Fix a bug with projections and the `aggregate_functions_null_for_empty` setting
ucasfl pushed a commit to ucasfl/ClickHouse that referenced this pull request Nov 30, 2022
amosbird added a commit to amosbird/ClickHouse that referenced this pull request May 15, 2023
setting (for query_plan_optimize_projection)

Fix a bug with projections and the aggregate_functions_null_for_empty
setting. This was already fixed in PR ClickHouse#42198 but got forgotten after
using query_plan_optimize_projection.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-bugfix Pull request with bugfix, not backported by default pr-must-backport Pull request should be backported intentionally. Use this label with great care!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

select max(date) from test_date always return -127593672
4 participants