Marton Greber 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 6: (26 comments) 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: based on > based on Kudu C++ client API Done http://gerrit.cloudera.org:8080/#/c/19690/5//COMMIT_MSG@10 PS5, Line 10: a table h > a non-unique Done 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 c Done 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 pha Newer IDEs, LSP based completers make heavy use of compile_commands.json. Users who are building up knowledge about Kudu client will most likely rely on autocomplete, suggestions to play around with the examples. That is why I left this in. On the other hand, if we are aiming to include only Kudu functional code, I can remove it, no problem. 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 exam Absolutely! I just wasn't sure on the positioning between supporting C++98 and using newer C++ features in new examples. Changed a couple places. The one place where I left raw pointers is "KuduPredicate* p = table->NewComparisonPredicate(...)", the predicates are used in "scanner.AddConjunctPredicate(...)" <- this takes ownership. Let me know if this works. http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@68 PS5, Line 68: > nit: drop the comma Done http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@81 PS5, Line 81: > specified Done http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@111 PS5, Line 111: : KUDU_RETURN_NOT_OK(ses > KUDU_RETURN_NOT_OK(session->Flush()) Done http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@138 PS5, Line 138: // 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 serve > How about: Done. Thanks for the clear formulation! http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@145 PS5, Line 145: KUDU_RETURN_NOT_OK(s > nit: move this inside the 'while' cycle? Done http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@172 PS5, Line 172: : KUDU_RETURN_NOT_OK(ses > nit: could be shorten to KUDU_RETURN_NOT_OK(session->Flush()) ? Done http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@179 PS5, Line 179: // 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 serve > How about: Done. Thanks for the clear formulation! http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@211 PS5, Line 211: : KUDU_RETURN_NOT_OK(ses > nit: KUDU_RETURN_NOT_OK(Flush()) Done http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@219 PS5, Line 219: FATAL > ERROR To run any of the C++ client apps, one has to provide the host address. IIUC there is no point in continuing in the execution, as it will just exit with a can't connect to kudu master error at the first point. I think FATAL is reasonable here, but if you think that even after this consideration, the ERROR is better suited, I'm happy to change over. http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@247 PS5, Line 247: t > the Done http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@253 PS5, Line 253: WHERE non_un > nit: drop this part Done http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@277 PS5, Line 277: > nit: drop the comma Done http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@278 PS5, Line 278: > nit: drop the comma Done http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@292 PS5, Line 292: > nit: drop the comma Done http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@293 PS5, Line 293: > nit: drop the comma Done http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@312 PS5, Line 312: > nit: drop the comma Done http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@314 PS5, Line 314: > nit: drop the comma Done http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@332 PS5, Line 332: > nit: drop the comma Done http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@346 PS5, Line 346: > nit: drop the comma Done http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@364 PS5, Line 364: > nit: drop the comma Done http://gerrit.cloudera.org:8080/#/c/19690/5/examples/cpp/non_unique_primary_key.cc@371 PS5, Line 371: t > the Done -- 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: 6 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: Tue, 11 Apr 2023 13:52:54 +0000 Gerrit-HasComments: Yes
