Skip to content

Fix bug in execution of TTL GROUP BY#25743

Merged
alesapin merged 7 commits intomasterfrom
fix_aggregation_ttl
Jul 7, 2021
Merged

Fix bug in execution of TTL GROUP BY#25743
alesapin merged 7 commits intomasterfrom
fix_aggregation_ttl

Conversation

@alesapin
Copy link
Copy Markdown
Member

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

Changelog category (leave one):

  • Bug Fix

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix bug in TTL with GROUP BY expression which refuses to execute TTL after first execution in part.

No backport because of changes in ttl.txt. Finally fixes test_replicated_ttl/test.py::test_ttl_compatibility.

@robot-clickhouse robot-clickhouse added the pr-bugfix Pull request with bugfix, not backported by default label Jun 27, 2021
@alesapin
Copy link
Copy Markdown
Member Author

No related failures.

@alesapin
Copy link
Copy Markdown
Member Author

@Mergifyio update
one more time

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jun 30, 2021

Command update: success

Branch has been successfully updated

@alesapin
Copy link
Copy Markdown
Member Author

alesapin commented Jul 1, 2021

Looks like in a very rare cases 01280_ttl_where_group_by completely ignores TTL

@alesapin
Copy link
Copy Markdown
Member Author

alesapin commented Jul 3, 2021

@Mergifyio update

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jul 3, 2021

Command update: success

Branch has been successfully updated

Copy link
Copy Markdown
Member

@CurtizJ CurtizJ left a comment

Choose a reason for hiding this comment

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

You set finished flag if isMinTTLExpired is true. Looks like it can lead to non-pruned partially expired parts. Shouldn't we use isMaxTTLExpired instead?

Comment on lines +114 to +115
if (!part.ttl_infos->hasAnyNonFinishedTTLs())
return false;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why we return false in this case?

@alesapin
Copy link
Copy Markdown
Member Author

alesapin commented Jul 7, 2021

No related failures.

@alesapin alesapin merged commit 4c85dae into master Jul 7, 2021
@alesapin alesapin deleted the fix_aggregation_ttl branch July 7, 2021 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix Pull request with bugfix, not backported by default pr-no-backport

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants