YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table ...................................................................... Patch Set 2: (11 comments) http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@330 PS1, Line 330: message ColumnPB { > What about encoding and compression? I add them. http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@341 PS1, Line 341: // lower bound value in format of string, include the value. : optional string in_lower_bound = 1; : // lower bound value in format of string, exclude the value. : optional string ex_lower_bound = 2; : // upper bound value in format of string, include the value. : optional string in_upper_bound = 3; : // upper bound value in format of string, exclude the value. : optional string ex_upper_bound = 4; > Why not model this as two string values and two enums, like in add_range_pa message RangeBoundPB { message BoundPB { // type of bound, "include" or "exclude" optional string bound_type = 1; // value of bound in type of string optional string bound_value = 2; } repeate BoundPB bounds = 1; } define like this? http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@350 PS1, Line 350: required string column = 1; > Shouldn't this be repeated, to match set_range_partition_columns()? Yes http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@351 PS1, Line 351: // range split bound. > Do we also want to support splits (i.e. range partition key values that def I will try to support. http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@371 PS1, Line 371: required string table_name = 1; > https://github.com/protocolbuffers/protobuf/issues/2497 explains why we don I know, thank you. http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: PS1: > In general Kudu adheres to the Google C++ Style Guide. Please reformat cont OK http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@877 PS1, Line 877: KuduValue* ParseValueOfType(const std::string& type, > Can't we use the existing ParseValueOfType method, with some modifications Yes. http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@911 PS1, Line 911: } else if (type == "DECIMAL") { > Why can't we reuse ConvertToKuduPartialRow? Yes. but we should change the first parameter of function ConvertToKuduPartialRow, like ConvertToKuduPartialRow(const vector<string>& columns, const vector<DataType>& types; const string& range_bound_str, KuduPartialRow* range_bound_partial_row) ? http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@1023 PS1, Line 1023: table_creator->set_range_partition_columns({}); > Use RETURN_NOT_OK instead of KUDU_CHECK_OK for cleaner failure behavior. OK http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@1031 PS1, Line 1031: > Nit: KuduPartialRow* Yes. http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@1263 PS1, Line 1263: .AddRequiredParameter({ kCompressionTypeArg, "Compression type of the column" }) : .Build(); > This doesn't make sense to me; why not just a single required parameter tha OK -- To view, visit http://gerrit.cloudera.org:8080/14306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 2 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: Fri, 27 Sep 2019 03:24:30 +0000 Gerrit-HasComments: Yes
