Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19272 )
Change subject: Auto-Incrementing Column, C++ client part1 ...................................................................... Patch Set 1: (15 comments) http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG@13 PS1, Line 13: similarly like nit: either 'similar to' or 'like' http://gerrit.cloudera.org:8080/#/c/19272/1//COMMIT_MSG@14 PS1, Line 14: only one column spec can be specified, Could you add the reasoning why is this restriction? 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: no primary key, nor nit: 'neither ... nor ...' http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/client-unittest.cc@80 PS1, Line 80: ->PrimaryKey() : ->NonUniquePrimaryKey(); Mind adding a sub-case with these two calls in different order? 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, bool include_auto_incrementing = true); It seems this change would break backwards ABI compatibility, so this isn't acceptable: https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B The idea is that the newer kudu_client dynamic library should be linkable with applications compiled with older versions of the headers/library. You can add a extra constructor with a new signature instead, I guess. 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: bool include_auto_incrementing = true The whole idea behind this ScanConfiguration class is to avoid adding many parameters into the constructor, but instead maintain the set of methods that change the configuration for the result. Please follow the suit and add corresponding method instead. http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.cc File src/kudu/client/scan_configuration.cc: http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.cc@66 PS1, Line 66: table Since that's the body of the class already, consider using the corresponding members instead of parameters? http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.cc@67 PS1, Line 67: auto ids Why not to use refrences instead of copying the vector here? http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.cc@70 PS1, Line 70: int idx Consider adding DCHECK_GE(idx, 0) to spot programming mistakes. http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.cc@76 PS1, Line 76: #pragma GCC diagnostic push : #pragma GCC diagnostic ignored "-Wunused-result" Instead, wrap the call below into CHECK_OK() http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/scan_configuration.cc@78 PS1, Line 78: this-> Why to add 'this->'? 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@585 PS1, Line 585: KuduColumnSpec* AutoIncrementing() It would be great to actually state what this method does and what it returns. http://gerrit.cloudera.org:8080/#/c/19272/1/src/kudu/client/schema.h@643 PS1, Line 643: SetNonUniquePrimaryKey > How about combining this functions with SetPrimaryKey(), change SetPrimaryK Wenzhe, that's not a good idea since it would break backwards ABI compatibility (see https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B for details). Kudu provides backward-compatible ABI with kudu_client library, so it's possible to make applications linked with prior version link with a dynamic library of a newer version. 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: return BitmapIsAllSet(isset_bitmap_, 0, schema_->num_key_columns()-1); nit: add spaces around '-' Also, add DCHECK() to make sure schema_->num_key_columns() is greater or equal to 1. 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: const std::string ColumnSchema::auto_incrementing_id = "auto_incrementing_id"; > Could we define the column name of auto_incrementing_id as constant and put I guess it makes sense to create a method for that: that's more flexible if we are ever to change the name for such column in future releases. Should this be a property of a particular Schema object? -- 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: 1 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: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Tue, 29 Nov 2022 22:25:47 +0000 Gerrit-HasComments: Yes
