Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18663 )
Change subject: WIP [c++ client] KUDU-2671 Custom hash schema alter table support ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/18663/1/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/18663/1/src/kudu/client/client.cc@1542 PS1, Line 1542: range_partition nit here and below: is it possible get rid of the variable and call new KuduTableCreator::KuduRangePartition() in-place? http://gerrit.cloudera.org:8080/#/c/18663/1/src/kudu/client/flex_partitioning_client-test.cc File src/kudu/client/flex_partitioning_client-test.cc: http://gerrit.cloudera.org:8080/#/c/18663/1/src/kudu/client/flex_partitioning_client-test.cc@275 PS1, Line 275: LESS_EQUAL It would be more convenient to have LESS here: by the common convention at least at the server side, the lower bound is inclusive and the upper bound is exclusive. Also, I'd expect to see rather CheckTableRowsNum(table_name, column, 0, 100, 100) than CheckTableRowsNum(table_name, column, 0, 100, 101) http://gerrit.cloudera.org:8080/#/c/18663/1/src/kudu/client/flex_partitioning_client-test.cc@1109 PS1, Line 1109: NO_FATALS(CheckTabletCount(kTableName, 5)) nit: probably it makes sense to check the count right after creating the table and before adding the range http://gerrit.cloudera.org:8080/#/c/18663/1/src/kudu/client/table_alterer-internal.h File src/kudu/client/table_alterer-internal.h: http://gerrit.cloudera.org:8080/#/c/18663/1/src/kudu/client/table_alterer-internal.h@66 PS1, Line 66: range_partition This is a raw pointer. Who is the owner? If that's this class, it must deallocate it in the destructor, no? Or better, wrap this into unique_ptr<> -- To view, visit http://gerrit.cloudera.org:8080/18663 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id4b1e306cca096d9479f06669cc22cc40d77fb42 Gerrit-Change-Number: 18663 Gerrit-PatchSet: 1 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Fri, 24 Jun 2022 05:52:34 +0000 Gerrit-HasComments: Yes
