Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19690 )
Change subject: KUDU-1945 Add C++ example for non-unique PK ...................................................................... Patch Set 5: (29 comments) Almost there! http://gerrit.cloudera.org:8080/#/c/19690/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19690/5//COMMIT_MSG@9 PS5, Line 9: C++ file based on Kudu C++ client API http://gerrit.cloudera.org:8080/#/c/19690/5//COMMIT_MSG@10 PS5, Line 10: non-unique a non-unique http://gerrit.cloudera.org:8080/#/c/19690/5//COMMIT_MSG@12 PS5, Line 12: STDOUT of the example: nit: update the snippet after addressing review feedback on the example's code http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/CMakeLists.txt File examples/cpp/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/CMakeLists.txt@21 PS5, Line 21: set(CMAKE_EXPORT_COMPILE_COMMANDS ON) Should this stay or this was used only during troubleshooting/debugging phase? http://gerrit.cloudera.org:8080/#/c/19690/1/examples/cpp/non_unique_primary_key.cc File examples/cpp/non_unique_primary_key.cc: http://gerrit.cloudera.org:8080/#/c/19690/1/examples/cpp/non_unique_primary_key.cc@67 PS1, Line 67: KuduSchemaBuilder b; : // Columns which are not uniquely identifiable can still be used as primary keys, by : // specifying them as non-unique primary key. : b.AddColumn("non_unique_key")->Type(KuduColumnSchema::INT32)->NotNull()->NonUniquePrimaryKey(); : b.AddColumn("int_val")->Type(KuduColumnSchema::INT32)->NotNull(); On a separate note: I wish we had something like AutoIncrementingColumnSchemaBuilder in C++ client. Do you think it's worth adding something like we have in Java client in C++ client as well? http://gerrit.cloudera.org:8080/#/c/19690/1/examples/cpp/non_unique_primary_key.cc@79 PS1, Line 79: vector<string> column_names; : // Use the non-unique key column for hash nit: could use initializers list for 'column_names' instead of push_back? http://gerrit.cloudera.org:8080/#/c/19690/1/examples/cpp/non_unique_primary_key.cc@377 PS1, Line 377: the http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc File examples/cpp/non_unique_primary_key.cc: PS5: Even if Kudu C++ client API is compatible with C++98, I think that new examples should use contemporary C++ standards, where std::unique_ptr and other niceties are available to avoid memory leaks and promote bug-free code. What do you think? http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@68 PS5, Line 68: , nit: drop the comma http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@81 PS5, Line 81: specifid specified http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@111 PS5, Line 111: Status s = session->Flush(); : KUDU_RETURN_NOT_OK(s); KUDU_RETURN_NOT_OK(session->Flush()) http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@138 PS5, Line 138: // With updates in general, one has to scan the table, to get the auto-incrementing value. : // As for update the entire set of key columns are necessary. How about: It's necessary to specify the entire set of key columns when updating a particular row. An auto-incrementing column is auto-populated at the server side, and one way to retrieve its values is scanning the table with a projection that includes the auto-incrementing column. http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@145 PS5, Line 145: KuduScanBatch batch; nit: move this inside the 'while' cycle? http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@172 PS5, Line 172: Status s = session->Flush(); : KUDU_RETURN_NOT_OK(s); nit: could be shorten to KUDU_RETURN_NOT_OK(session->Flush()) ? http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@179 PS5, Line 179: // With deletes in general, one has to scan the table, to get the auto-incrementing value. : // As for delete the entire set of key columns are necessary. How about: It's necessary to specify the entire set of key columns when updating a particular row. An auto-incrementing column is auto-populated at the server side, and one way to retrieve its values is scanning the table with a projection that includes the auto-incrementing column. http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@211 PS5, Line 211: Status s = session->Flush(); : KUDU_RETURN_NOT_OK(s); nit: KUDU_RETURN_NOT_OK(Flush()) http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@219 PS5, Line 219: FATAL ERROR Otherwise, it would just crash for no reason with FATAL? http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@247 PS5, Line 247: a the http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@253 PS5, Line 253: from table, nit: drop this part http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@277 PS5, Line 277: , nit: drop the comma http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@278 PS5, Line 278: , nit: drop the comma http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@292 PS5, Line 292: , nit: drop the comma http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@293 PS5, Line 293: , nit: drop the comma http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@312 PS5, Line 312: , nit: drop the comma http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@314 PS5, Line 314: , nit: drop the comma http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@332 PS5, Line 332: , nit: drop the comma http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@346 PS5, Line 346: , nit: drop the comma http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@364 PS5, Line 364: , nit: drop the comma http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@371 PS5, Line 371: a the -- To view, visit http://gerrit.cloudera.org:8080/19690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8c6be6bece56788dc858468d1fcccff6955836ec Gerrit-Change-Number: 19690 Gerrit-PatchSet: 5 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: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Sun, 09 Apr 2023 16:48:09 +0000 Gerrit-HasComments: Yes
