Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/21496 )
Change subject: [tool] Add '--columns' param to 'table list' ...................................................................... Patch Set 8: (7 comments) 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) -> \ : string { return table_info.x; } : : #define GET_NUM_PROPERTY(x) \ : [](const KuduClient::Data::TableInfo& table_info) -> \ : string { return std::to_string(table_info.x); } > style nit: if using local macros like these, add corresponding #undef after Done http://gerrit.cloudera.org:8080/#/c/21496/7/src/kudu/tools/tool_action_table.cc@325 PS7, Line 325: unord > nit: drop 'std::' prefix since there is corresponding 'using' directive in Done 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 na Done http://gerrit.cloudera.org:8080/#/c/21496/7/src/kudu/tools/tool_action_table.cc@336 PS7, Line 336: : for (const auto& tinfo : tables_info) { : vector<string> values; : for (const auto& column : columns) { : s > nit: if index isn't required, prefer iterating over the elements of the con Done http://gerrit.cloudera.org:8080/#/c/21496/7/src/kudu/tools/tool_action_table.cc@370 PS7, Line 370: (!MatchesAnyPattern(table_filte > Is this still needed after switching to the approach with the '--columns' f ah, good catch, no it is not needed, removed http://gerrit.cloudera.org:8080/#/c/21496/7/src/kudu/tools/tool_action_table.cc@917 PS7, Line 917: ListTables(const R > nit: please use FLAGS_columns.empty() instead Done http://gerrit.cloudera.org:8080/#/c/21496/7/src/kudu/tools/tool_action_table.cc@1936 PS7, Line 1936: .AddOptionalParameter("show_hash_partition_info") : .AddOptionalParameter("show_table_info") : .AddOptionalParameter("list_table_output_format") : .AddOptionalParameter( : "columns", > nit: fix alignment (check other CLI actions for reference) Done -- 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: 8 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 12:41:54 +0000 Gerrit-HasComments: Yes
