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 2: (15 comments) http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG@7 PS1, Line 7: KUDU > Add the Jira to keep track of the patches. Done http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG@13 PS1, Line 13: like PrimaryKe > nit: either 'similar to' or 'like' Done http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG@14 PS1, Line 14: only one column can have the NonUnique > Could you add the reasoning why is this restriction? What I ment here is like with PrimaryKey(), only one column can be marked with this ColumnSpec in KuduSchemaBuilder. This is the case with NonUniquePrimaryKey() as well. Changed the commit message to explain this. http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG@17 PS1, Line 17: set fu > don't Done http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client-unittest.cc File src/kudu/client/client-unittest.cc: http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client-unittest.cc@63 PS1, Line 63: neither primary key > nit: 'neither ... nor ...' Done http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client-unittest.cc@80 PS1, Line 80: b.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull() : ->PrimaryKey() > Mind adding a sub-case with these two calls in different order? Done http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client.h@2763 PS1, Line 2763: explicit KuduScanner(KuduTable* table); > It seems this change would break backwards ABI compatibility, so this isn't Done http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client.h@3226 PS1, Line 3226: eration result status. > If auto_increment_id column is added in token, the scanner have to read au This is working. To confirm, I added test see: scan_token-test.cc http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.h File src/kudu/client/scan_configuration.h: http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.h@53 PS1, Line 53: > The whole idea behind this ScanConfiguration class is to avoid adding many Thank you for the explanation, understood. I removed this approach. Feature-wise, users can keep or discard the auto-incrementing column by using one of the SetProjectedColumn functions, or with scan tokens. Now understanding ABI requirements, and the weight with adding stuff to the client: do we want to add convenience function to KuduScanner, to handle auto-incrementing column projection? Like adding a function KuduScanner::IncludeAutoIncrementingColumn(bool include=true). http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@497 PS1, Line 497: SetNonUniquePrim > nit: SetNonUniquePrimaryKey() Done http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@499 PS1, Line 499: unique > nit: unique Done http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@582 PS1, Line 582: increment > nit: Let's call it auto-incrementing everywhere to keep it uniform. Done http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@585 PS1, Line 585: // Users are not expected to acces > It would be great to actually state what this method does and what it retur Added detailed comments here. http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/common/partial_row.cc File src/kudu/common/partial_row.cc: http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/common/partial_row.cc@888 PS1, Line 888: DCHECK(schema_->num_key_columns() >= 1); > nit: add spaces around '-' Done http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/common/schema.cc File src/kudu/common/schema.cc: http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/common/schema.cc@198 PS1, Line 198: Schema::Schema(const Schema& other) > This should be property of KuduSchema object. Moved this constant to: const string KuduSchema::auto_incrementing_id() . I added this as static function. -- 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: 2 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: Thu, 01 Dec 2022 18:09:30 +0000 Gerrit-HasComments: Yes
