Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client ...................................................................... Patch Set 19: Code-Review+1 (10 comments) Just a few nits, overall looks good to me. http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/scan_token-test.cc File src/kudu/client/scan_token-test.cc: http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/scan_token-test.cc@338 PS19, Line 338: const KuduScanToken* style nit: consider using const reference here instead -- in most cases in Kudu codebase, constant parameters are passed by reference, not by pointer http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/scan_token-test.cc@341 PS19, Line 341: shared_ptr<KuduClient> client; : CHECK_OK(cluster_->CreateClient(nullptr, &client)); Why not to use already existing 'client_' member field here? Would be great to add a short blurb to explain the reasoning behind. http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.h@798 PS19, Line 798: Utility function to return the actual name of the auto incrementing column I guess this is only for the time when a table with implicitly added column is created? In other words, that's the default name for the auto-incrementing column, IIUC. We don't have any restriction in place to prohibit changing the name of the auto-incrementing column later on using AlterTable, right? http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.h@867 PS19, Line 867: auto_incrementing_col_name style nit: this should either follow the naming convention for regular field (like 'schema_' below) and end with underscore or the convention of naming static constant/constexpr static fields, where it would be something like kAutoIncColName or something similar. http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc@575 PS19, Line 575: vector<KuduColumnSchema>& style nit: in most of the Kudu code, the "out" and "inout" parameters are usually passed as a pointer http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc@589 PS19, Line 589: std::string Name; : KuduColumnSchema::DataType Type; style nit: the name of these fields should start with non-capital letters Shouldn't they be static constexpr ones? Also, make this private section explicit (i.e. add the 'private:' tag) and move it to be after all the methods and fields in the 'public:' section. http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc@594 PS19, Line 594: style nit: add two extra spaces before the column http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc@609 PS19, Line 609: bool key_cols_unique; Should this field be initialized in the constructor? http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc@682 PS19, Line 682: nit: remove this extra line? http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/common/schema.cc File src/kudu/common/schema.cc: http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/common/schema.cc@a293 PS19, Line 293: : : : : : : : : : : : Does it make sense to keep the logic to enforce the invariants for auto-incrementing column in the form of corresponding DCHECK() macros to spot programming mistakes? -- To view, visit http://gerrit.cloudera.org:8080/19272 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic133e3d44cc56c8351e33d95b523ed7b6b13617b Gerrit-Change-Number: 19272 Gerrit-PatchSet: 19 Gerrit-Owner: Marton Greber <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Wed, 18 Jan 2023 00:55:00 +0000 Gerrit-HasComments: Yes
