Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21496 )
Change subject: [tool] Add '--columns' param to 'table list' ...................................................................... Patch Set 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/21496/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21496/3//COMMIT_MSG@7 PS3, Line 7: dd '--columns' param to 'table list' > I made the recommended changes. Should I already mark the '--show_table_inf Right: marking deprecated can be done now, but it's necessary to the functionality available for about one release. I'd recommend marking the '--show_table_info' as deprecate in a separate changelist, though. http://gerrit.cloudera.org:8080/#/c/21496/7/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/21496/7/src/kudu/tools/tool_action_table.cc@317 PS7, Line 317: #define GET_PROPERTY(x) \ : [](const KuduClient::Data::TableInfo& table_info) -> \ : std::string { return table_info.x; } : : #define GET_NUM_PROPERTY(x) \ : [](const KuduClient::Data::TableInfo& table_info) -> \ : std::string { return std::to_string(table_info.x); } style nit: if using local macros like these, add corresponding #undef after the code that uses them http://gerrit.cloudera.org:8080/#/c/21496/7/src/kudu/tools/tool_action_table.cc@325 PS7, Line 325: std:: nit: drop 'std::' prefix since there is corresponding 'using' directive in the beginning of the file http://gerrit.cloudera.org:8080/#/c/21496/7/src/kudu/tools/tool_action_table.cc@326 PS7, Line 326: mapping = { : {"id", GET_PROPERTY(id)}, : {"name", GET_PROPERTY(table_name)}, : {"num_tablets", GET_NUM_PROPERTY(num_tablets)}, : {"num_replicas", GET_NUM_PROPERTY(num_replicas)}, : {"live_row_count", GET_NUM_PROPERTY(live_row_count)}, : }; As with other kudu CLI tools supporting the `--columns` flag, the column names are supposed to be case-insensitive. It seems that's not the case in this implementation -- please fix it. http://gerrit.cloudera.org:8080/#/c/21496/7/src/kudu/tools/tool_action_table.cc@336 PS7, Line 336: for (int i = 0; i < columns.size(); i++) { : auto getter = mapping.find(columns[i]); : if (getter == mapping.end()) { : return Status::InvalidArgument(Substitute("Invalid column name: $0", columns[i])); : } nit: if index isn't required, prefer iterating over the elements of the container like if (const auto& column : columns) ... http://gerrit.cloudera.org:8080/#/c/21496/7/src/kudu/tools/tool_action_table.cc@370 PS7, Line 370: table_info_pb->set_id(tinfo.id); Is this still needed after switching to the approach with the '--columns' flag? http://gerrit.cloudera.org:8080/#/c/21496/7/src/kudu/tools/tool_action_table.cc@917 PS7, Line 917: FLAGS_columns != "" nit: please use FLAGS_columns.empty() instead http://gerrit.cloudera.org:8080/#/c/21496/7/src/kudu/tools/tool_action_table.cc@1936 PS7, Line 1936: "columns", : string(""), : string("Comma-separated list of table info fields to " : "include in output. \nPossible values: id, name, live_row_count, " : "num_tablets, num_replicas")) nit: fix alignment (check other CLI actions for reference) -- To view, visit http://gerrit.cloudera.org:8080/21496 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I324b920e6feb6139e7d884e3cf08069b0cb922a4 Gerrit-Change-Number: 21496 Gerrit-PatchSet: 7 Gerrit-Owner: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Anonymous Coward (763) Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Tue, 18 Jun 2024 01:47:23 +0000 Gerrit-HasComments: Yes
