Abhishek Chennaka 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? Done 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 'parti Done 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? Thanks for catching it. Fixed it. 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: Yea not sure what is missing here. It was as per the suggestion here -> http://jenkins.kudu.apache.org/job/kudu-gerrit/25847/BUILD_TYPE=IWYU/console I re-added it now. 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? Yes, it's not needed. 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 mayb That seems to have been added when KuduTableAlterer::KuduRangePartition() was being implemented. Thanks for catching it. 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 Done 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 in Done 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 a Done -- 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 04:32:55 +0000 Gerrit-HasComments: Yes
