Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/18642 )
Change subject: KUDU-2671 update partition schema in catalog when dropping range ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/18642/1/src/kudu/common/partition-test.cc File src/kudu/common/partition-test.cc: http://gerrit.cloudera.org:8080/#/c/18642/1/src/kudu/common/partition-test.cc@1898 PS1, Line 1898: ASSERT_EQ(0, ps.ranges_with_hash_schemas().size()); nit: check helper map size before and after as well. http://gerrit.cloudera.org:8080/#/c/18642/1/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/18642/1/src/kudu/common/partition.cc@1738 PS1, Line 1738: continue; I know there's a check in catalog manager but do you think it's worth adding a check in these for loops to ensure that this if condition actually gets triggered? http://gerrit.cloudera.org:8080/#/c/18642/1/src/kudu/common/partition.cc@1744 PS1, Line 1744: updated_array.size() Shouldn't this be size as well? updated_array.size() will be (size - 1) assuming the above for loop operates correctly. When the if condition gets triggered here, then the size of updated_mapping will be (size - 2) which doesn't seem right. Maybe we could add some checks on the size of the respective fields before and after these for loops. http://gerrit.cloudera.org:8080/#/c/18642/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/18642/1/src/kudu/master/catalog_manager.cc@2699 PS1, Line 2699: } else if (step.type() == AlterTableRequestPB::DROP_RANGE_PARTITION) { To summarize, for adding a range you just updated the partition schema protobuf directly in the catalog but for dropping a range you updated the partition schema object then serialized the object into the protobuf and updated it in the catalog. Is the difference in approaches due to the lack of a remove/drop method for the repeated field of protobuf? -- To view, visit http://gerrit.cloudera.org:8080/18642 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib78afdd1a358751dca43f564c5d8e69191f165d4 Gerrit-Change-Number: 18642 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Tue, 21 Jun 2022 21:55:06 +0000 Gerrit-HasComments: Yes
