-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[BugFix] fix expensive Partition.equals #34961
[BugFix] fix expensive Partition.equals #34961
Conversation
@@ -1429,7 +1432,7 @@ private void addPartitions(Database db, String tableName, List<PartitionDesc> pa | |||
checkPartitionType(partitionInfo); | |||
|
|||
// update partition info | |||
updatePartitionInfo(partitionInfo, partitionMap, existPartitionNameSet, addPartitionClause, olapTable); | |||
updatePartitionInfo(partitionInfo, newPartitions, existPartitionNameSet, addPartitionClause, olapTable); | |||
|
|||
colocateTableIndex.updateLakeTableColocationInfo(olapTable, true /* isJoin */, | |||
null /* expectGroupId */); |
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 most risky bug in this code is:
Incorrect usage of Partition
and PartitionDesc
pairs after changing from a Map
to a List<Pair<Partition, PartitionDesc>>
.
You can modify the code like this:
// Wherever partitionMap was used, replace it with partitionList using appropriate methods
// For example, you would access the entries with something like this:
for (Pair<Partition, PartitionDesc> entry : partitionList) {
Partition partition = entry.first;
PartitionDesc partitionDesc = entry.second;
// Use partition and partitionDesc as needed
}
Make sure to review the entire code and replace all usages of the old partitionMap
with the new partitionList
, accessing the items through their .first
and .second
properties.
Signed-off-by: Murphy <mofei@starrocks.com>
efb33e5
to
62b27e3
Compare
Kudos, SonarCloud Quality Gate passed!
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 22 / 24 (91.67%) file detail
|
@Mergifyio backport branch-3.2 |
@Mergifyio backport branch-3.1 |
✅ Backports have been created
|
✅ Backports have been created
|
Signed-off-by: Murphy <mofei@starrocks.com> (cherry picked from commit 5cc830c) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/catalog/Partition.java # fe/fe-core/src/main/java/com/starrocks/server/LocalMetastore.java
Signed-off-by: Murphy <mofei@starrocks.com> (cherry picked from commit 5cc830c)
Signed-off-by: Murphy <mofei@starrocks.com> (cherry picked from commit 5cc830c)
Signed-off-by: Murphy <mofei@starrocks.com> (cherry picked from commit 5cc830c) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/catalog/Partition.java # fe/fe-core/src/main/java/com/starrocks/server/LocalMetastore.java
Why I'm doing:
Partition.equals
is too expensive to used as map keyWhat I'm doing:
Partition
as Map keyPartition.id
tohashCode
Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: