YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14273 )
Change subject: add a tool to create table ...................................................................... Patch Set 5: (4 comments) 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. Yes, this function is too long. I will try to decompose it. 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. Yes, I modify it. 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 sche I'm Sorry, I don't understand this place. The JSON schema is provided to the user. Here parse it and use kudu::client::KuduTableCreator to create table. Finally the JSON will transform to CreateTableRequestPB, then send to master. Here I need provide a protobuf like CreateClusterRequestPB for user? using protobuf instead of JSON? 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 ta At first I want to define a JSON object, but the object is too long, it's very easy to write wrongly, and it's troublesome to revise, so I change to use a json file as "src/kudu/tools/create_table_file". Of course it's easy to modify. Or maybe both support? -- 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-Reviewer: YangSong <[email protected]> Gerrit-Comment-Date: Mon, 23 Sep 2019 03:18:32 +0000 Gerrit-HasComments: Yes
