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 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/18663/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18663/3//COMMIT_MSG@18 PS3, Line 18: off nit: maybe, drop the 'off' part? http://gerrit.cloudera.org:8080/#/c/18663/3/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/18663/3/src/kudu/client/client.h@1910 PS3, Line 1910: KuduTableAlterer* AddRangePartition( Please add a note that the alterer takes the ownership of the passed 'partition' raw pointer. It's important to document since the signature of this method doesn't provide explicit semantics on the ownership. It maybe something like: @note The table alterer takes ownership of the partition object. http://gerrit.cloudera.org:8080/#/c/18663/3/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/18663/3/src/kudu/client/client.cc@1558 PS3, Line 1558: partition->data_->lower_bound_->schema() != partition->data_->lower_bound_->schema() Not sure how this can ever be false. Is there a typo here? http://gerrit.cloudera.org:8080/#/c/18663/3/src/kudu/client/flex_partitioning_client-test.cc File src/kudu/client/flex_partitioning_client-test.cc: PS3: It seems IWYU isn't happy about this file yet: 13:02:59 >>> Fixing #includes in '/home/jenkins-slave/workspace/kudu-master/2/src/kudu/client/flex_partitioning_client-test.cc' 13:02:59 @@ -30,6 +30,7 @@ 13:02:59 13:02:59 #include "kudu/client/client.h" 13:02:59 #include "kudu/client/scan_batch.h" 13:02:59 +#include "kudu/client/scan_predicate.h" 13:02:59 #include "kudu/client/schema.h" 13:02:59 #include "kudu/client/value.h" 13:02:59 #include "kudu/client/write_op.h" 13:02:59 IWYU would have edited 1 files on your behalf. http://gerrit.cloudera.org:8080/#/c/18663/3/src/kudu/client/flex_partitioning_client-test.cc@277 PS3, Line 277: vector<string> rows; Is this ever used in this code? http://gerrit.cloudera.org:8080/#/c/18663/3/src/kudu/client/table_alterer-internal.h File src/kudu/client/table_alterer-internal.h: http://gerrit.cloudera.org:8080/#/c/18663/3/src/kudu/client/table_alterer-internal.h@41 PS3, Line 41: typedef std::vector<HashDimension> HashSchema; Just curious where this is used? I don't see it used in this file, so maybe move this into corresponding .cc file that needs this typedef? http://gerrit.cloudera.org:8080/#/c/18663/3/src/kudu/client/table_alterer-internal.h@61 PS3, Line 61: which needs to be added or dropped nit: to add or drop http://gerrit.cloudera.org:8080/#/c/18663/3/src/kudu/client/table_alterer-internal.cc File src/kudu/client/table_alterer-internal.cc: http://gerrit.cloudera.org:8080/#/c/18663/3/src/kudu/client/table_alterer-internal.cc@180 PS3, Line 180: range_partition->data_ Here and below: could use the newly introduced 'partition_data' variable instead? http://gerrit.cloudera.org:8080/#/c/18663/3/src/kudu/client/table_alterer-internal.cc@204 PS3, Line 204: s.range_partition->data_ nit for here and below: could use a var for s.range_partition->data_ here as well -- 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: 3 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 30 Jun 2022 01:06:35 +0000 Gerrit-HasComments: Yes
