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 stupid case of intersecting parts #58482
Conversation
This is an automated comment for commit bc1c05e with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page Successful checks
|
/// We can only create a covering part for a blocks range that starts with 0 (otherwise we may get "intersecting parts"). | ||
/// Maybe we could do it by incrementing mutation version to get a name for the empty covering part, | ||
/// but it's okay to simple avoid creating it for DROP PART | ||
bool is_drop_part = drop_range.min_block; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is difficult to understand.
Here, I can read that "the range means 'drop part' if the min block number is not zero" which contradicts the comment.
Also, why it's "drop part", not "drop partition"?
Moreover, it's unclear when we have a zero block number. I thought block numbers start from one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the range means 'drop part' if the min block number is not zero" which contradicts the comment
Yes, that's correct, but it does not contradict the comment. What do you find contradictory?
Also, why it's "drop part", not "drop partition"?
Because we have a problem with "drop part", not with "drop partition"
Moreover, it's unclear when we have a zero block number. I thought block numbers start from one.
Block numbers start from 1 for MergeTree. For ReplicatedMergeTree they start from 0. So we have 0 for the first block in partition, and for all DROP_RANGEs for the whole partition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Block numbers start from 1 for MergeTree. For ReplicatedMergeTree they start from 0.
This is non obvious and has to be clarified in comments.
Would you mind also changing the first block number to 0 for non-replicated MergeTree?
What do you find contradictory?
If we drop a part with block number zero, it will be a "drop part" operation, but bool is_drop_part
will be false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind also changing the first block number to 0 for non-replicated MergeTree?
I can change it in a separate PR, but it will require updating a few hundred tests...
If we drop a part with block number zero, it will be a "drop part" operation, but bool is_drop_part will be false.
Yes, you're right. But it's not a problem because we don't have any parts on the left of the range, so we don't need to distinguish "drop part" in this case. I will update the comment and change the flag name
9925f00
to
0721e97
Compare
0721e97
to
9f50157
Compare
…24435aa3ccd39110a164f01ca084cd Cherry pick #58482 to 23.11: Fix a stupid case of intersecting parts
…24435aa3ccd39110a164f01ca084cd Cherry pick #58482 to 23.12: Fix a stupid case of intersecting parts
Backport #58482 to 23.11: Fix a stupid case of intersecting parts
Backport #58482 to 23.12: Fix a stupid case of intersecting parts
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix
Part ... intersects part ...
error that might occur inReplicatedMergeTree
when the server was restarted just after [automatically] dropping [an empty] part and adjacent parts were merged. The bug was introduced in #56282