ZhangYao has posted comments on this change. ( http://gerrit.cloudera.org:8080/14292 )
Change subject: [tools] Add get table statistics for CLI tools ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/14292/1/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/14292/1/src/kudu/client/client.h@977 PS1, Line 977: std::string ToString() const; > ToString() is more idiomatic in Kudu (see KuduScanner). Done http://gerrit.cloudera.org:8080/#/c/14292/1/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/14292/1/src/kudu/tools/tool_action_table.cc@870 PS1, Line 870: KuduTableStatistics *table_statistics; > Nit: what's the extra scope for? Not like table_statistics does anything fa Yeah, it's of no use. I forgot to remove it. http://gerrit.cloudera.org:8080/#/c/14292/1/src/kudu/tools/tool_action_table.cc@885 PS1, Line 885: .Description("Delete a table") : .AddRequiredParameter({ kMasterAddressesArg, kMasterAddressesArgDesc }) : .AddRequiredParameter({ kTableNameArg, "Name of the table to delete" }) : .AddOptionalParameter("modify_external_catalogs") : .Build(); : > I think "analyze" promises much more than what this actually does. How abou Make sense, make it simple and more understandable. Maybe when we have more metrics about table we can refactor it as "analyze" just like in Hive. -- To view, visit http://gerrit.cloudera.org:8080/14292 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie946a29ca5731ef72270d4ef870dd5919edaf869 Gerrit-Change-Number: 14292 Gerrit-PatchSet: 2 Gerrit-Owner: ZhangYao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: ZhangYao <[email protected]> Gerrit-Comment-Date: Wed, 25 Sep 2019 03:10:51 +0000 Gerrit-HasComments: Yes
