Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14273 )
Change subject: add a tool to create table ...................................................................... Patch Set 5: (6 comments) Could you add tests to kudu-tool-test to exercise the new functionality? Given the various levers involved in table creation, maybe you could add a "fuzz" test that randomly chooses a different (valid) configuration to exercise. http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc@869 PS5, Line 869: Status CreateTable(const RunnerContext& context) { This function is enormous and could stand to be decomposed via helpers. http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc@870 PS5, Line 870: static unordered_map<string, KuduColumnSchema::DataType> data_type_map = : {{"INT8", KuduColumnSchema::INT8}, : {"INT16", KuduColumnSchema::INT16}, : {"INT32", KuduColumnSchema::INT32}, : {"INT64", KuduColumnSchema::INT64}, : {"STRING", KuduColumnSchema::STRING}, : {"BOOL", KuduColumnSchema::BOOL}, : {"FLOAT", KuduColumnSchema::FLOAT}, : {"DOUBLE", KuduColumnSchema::DOUBLE}, : {"BINARY", KuduColumnSchema::BINARY}, : {"UNIXTIME_MICROS", KuduColumnSchema::UNIXTIME_MICROS}, : {"DECIMAL", KuduColumnSchema::DECIMAL}}; We already have the opposite of this in KuduColumnSchema::DataTypeToString. Perhaps you could enhance that code to allow for type -> string and string -> type lookups? http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc@897 PS5, Line 897: RETURN_NOT_OK(reader.ExtractString(reader.root(), "TableName", &table_name)); Looks like you're working against a predefined (even if implicit) JSON schema. Could you define it as a protobuf, then use protobuf helper functions to serialize/deserialize? See tool.proto and tool_action_common.cc (ControlShellProtocol) for an example. http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc@1392 PS5, Line 1392: .AddRequiredParameter({ kCreateFileNameArg, "Name of the table file" }) I don't think we should force clients to use a file. Instead, let's just take a JSON object as the argument; clients who wish to use a file can use shell redirection to pipe the file contents into the call. http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/util/jsonreader.h File src/kudu/util/jsonreader.h: http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/util/jsonreader.h@95 PS5, Line 95: // Judge the field whether exist, return true if exist, if not return false How about "Returns true if the field exists, false otherwise"? http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/util/jsonreader.h@97 PS5, Line 97: const char* field); Nit: fix indentation (should line up with 'const rapidjson...' from the line before). -- To view, visit http://gerrit.cloudera.org:8080/14273 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 Gerrit-Change-Number: 14273 Gerrit-PatchSet: 5 Gerrit-Owner: YangSong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 20 Sep 2019 23:38:37 +0000 Gerrit-HasComments: Yes
