Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: KUDU-1945 Auto-Incrementing Column, C++ client ...................................................................... Patch Set 20: (9 comments) http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/client-unittest.cc File src/kudu/client/client-unittest.cc: http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/client-unittest.cc@267 PS19, Line 267: > I think Column name 'auto_incrementing_id' should be reserved column name f In the new revision, I made the auto incrementing column name a reserved name. Users can't use it to name columns. I added tests 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: > style nit: consider using const reference here instead -- in most cases in Thanks for the explanation. Done. http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/scan_token-test.cc@341 PS19, Line 341: CHECK_OK(token.Serialize(&serialized_token)); : KuduScanner* scanner_ptr; > Why not to use already existing 'client_' member field here? Would be grea We can totally use existing 'client_' member field. Removed this unnecessary part. 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 > Yes, there will be a followup patch on the server side to restrict alter ta Since this came up during review, I took some time to add restrictions to the alter table operations in this patch: -users can't drop the auto incrementing column, -users can't rename the auto incrementing column, -users can't add a new column with the reserved auto incrementing column name. Moreover added checks to make the auto incrementing column a reserved column name, which can't be used by users to name columns. http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.h@867 PS19, Line 867: > style nit: this should either follow the naming convention for regular fiel Done 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: > style nit: in most of the Kudu code, the "out" and "inout" parameters are u Thanks for explaining! Done. http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc@589 PS19, Line 589: class KuduAutoIncrementingColumnSpecBuilder { : public: > style nit: the name of these fields should start with non-capital letters Done x3. Small update on the GetAutoIncrementingColumnName() function. Previously it was part of KuduSchema. There were places in common/schema where I needed this method. But to link from client->common introduces circular dependency. So I moved the column name definition into common/schema. Then passed it 'up' to client/schema. (And if I understand the architecture correctly this is the purpose of common and client separation). Without this change I couldn't make this name constexpr, as the GetAutoIncrementingColumnName() would lie in client/schema which can't use constexpr for the old cpp version compatibility. (at least the client example failed). http://gerrit.cloudera.org:8080/#/c/19272/19/src/kudu/client/schema.cc@594 PS19, Line 594: ~K > style nit: add two extra spaces before the column Done 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-inc Thinking more about it, yes I think it makes sense. Users have the SchemaBuilder API, however internally there are various ways, where these requires might not be set, eg creating custom tests. Like: Schema schema = Schema({ ColumnSchema("c0", INT32), ColumnSchema(Schema::GetAutoIncrementingColumnName(), INT64, false,false, true), },2); This is from the auto_incrementing-itest.cc The Schema constructor does not use the builder api, which utilises the AutoIncrementingColumnSpecBuilder, which pins down the properties. After similar consideration, I added DCHECKs for the checks in this file. Thank you for noticing this! -- 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: 20 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: Fri, 20 Jan 2023 22:20:35 +0000 Gerrit-HasComments: Yes
