Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13976 )
Change subject: [tools] Add table tools to delete column and alter column ...................................................................... Patch Set 7: (18 comments) http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/client/schema.h@583 PS6, Line 583: bool HasColumn(const std::string& col_name, KuduColumnSchema* col_schema) const; > Maybe rename to "HasColumn"? Done http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/client/schema.h@583 PS6, Line 583: e, KuduColumnSchem > Nit: "KuduColumnSchema* " Done http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@18 PS6, Line 18: #include <algorithm> > Nit: prefer cstdlib over this. Done http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@86 PS6, Line 86: "Also check for the existence of the row on the leader replica of " : "the tablet. If found, the full row will be > Good question. We have implemented "sub-modes" (i.e. mode within another mo Done http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@676 PS6, Line 676: Status DropRangePartition(const RunnerContext& context) { > Don't need std:: prefix for std::string. Done http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@677 PS6, Line 677: return ModifyRangePartition(context, PartitionAction::DROP); > Could this be defined statically and using an initializer_list: Done http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@695 PS6, Line 695: "Only one default value should be provided, but we got $0 value", : std::to_string(values.size()))); > Nit: indentation Done http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@698 PS6, Line 698: switch (type) { > But for STRING/BINARY columns, isn't the empty string a valid value? If users want to set default value of a STRING/BINARY column to an empty string , they could remove_default for the column. I finally set the default_value argument as a JSON array on account of another problem. That is if we want to set_default for a INT column to a negative number, such as '-1', the command line tool will parse it as a flag. http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@706 PS6, Line 706: reader.ExtractInt64(values[0], /*field=*/nullptr, &int_value), : Substitute("unable to parse value for column type $0", : KuduColumnSchema::DataTypeToString(type))); : *value = KuduValue::FromInt(int_value); : b > Use safe_strto64() here instead. And use the other safe_ functions below in Done http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@716 PS6, Line 716: reader.ExtractString(values[0], /*field=*/nullptr, &str_value), > Isn't this guaranteed by L697? Done http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@720 PS6, Line 720: break; : } : case KuduColumnSchema::DataType::BOOL: { : bool bool_value; : RETURN_NOT_OK_PREPEND( > Use boost::iequals() for the comparison to make it case-insensitive. Here I parse it as a JSON value. http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@736 PS6, Line 736: KuduColumnSchema::DataTypeToString(type))); > Shouldn't this be DataType::DECIMAL? Same above for UNIXTIME_MICROS. Done http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@737 PS6, Line 737: *value = KuduValue::FromFloat(double_value); : break; : } : case KuduColumnSchema::DataType::DOUBLE: { : double double_value; : RETURN_NOT_OK_PREPEND( : reader.ExtractDouble(values[0], /*field=*/n > These can be combined. Done http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@750 PS6, Line 750: default: > Same comment about static definition and an initializer_list. Done http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@759 PS6, Line 759: Status ColumnSetDefault(const RunnerContext& context) { : const string& table_name = FindOrDie(context.required_args, kTableNameArg); > This is duplicated. Done http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@779 PS6, Line 779: > Same. Done http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@1021 PS6, Line 1021: .Build(); > Indentation here and below doesn't match that of the existing Actions. Done http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@1024 PS6, Line 1024: > alter Done -- To view, visit http://gerrit.cloudera.org:8080/13976 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I228340e46fe48ffc782c4c7346f890444b8c550f Gerrit-Change-Number: 13976 Gerrit-PatchSet: 7 Gerrit-Owner: Yifan Zhang <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Thu, 15 Aug 2019 09:36:47 +0000 Gerrit-HasComments: Yes
