Adar Dembo 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 6: (18 comments) I didn't review the test changes. 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 Column(const std::string& col_name, KuduColumnSchema *col_schema) const; Maybe rename to "HasColumn"? http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/client/schema.h@583 PS6, Line 583: KuduColumnSchema * Nit: "KuduColumnSchema* " 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 <stdlib.h> Nit: prefer cstdlib over this. http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@86 PS6, Line 86: DEFINE_string(alter_column_args, "", : "Args of alter operation on the column."); This isn't very intuitive, which makes me think that the various "alter column" operations should be implemented as individual tools. It's more code to set up the various Actions, but it'll allow you to customize the help properly and better validate the input. Plus with some decomposition there shouldn't be any duplicated code in the core logic. http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@676 PS6, Line 676: Status ParseAlterType(const std::string& str_alter_type, AlterType* alter_type) { Don't need std:: prefix for std::string. http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@677 PS6, Line 677: // Build a alter_type_map from string to AlterType. Could this be defined statically and using an initializer_list: static unordered_map<string, AlterType> kAlterTypeMap = ({{ "...", SET_DEFAULT } { ..., ... } }); http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@695 PS6, Line 695: KuduColumnSchema::DataType type, : KuduValue** value) { Nit: indentation http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@698 PS6, Line 698: return Status::InvalidArgument("The set_default value cannot be empty."); But for STRING/BINARY columns, isn't the empty string a valid value? Would it be helpful if the default value argument was JSON-encoded? Then it'd be easier to express empty strings. http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@706 PS6, Line 706: int64_t int64_value = atoi64(str_value); : if (std::to_string(int64_value) != str_value) { : return Status::InvalidArgument(Substitute( : "Failed to parse value from $0 to a int value.", str_value)); : } Use safe_strto64() here instead. And use the other safe_ functions below in the FLOAT and DOUBLE conversions. http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@716 PS6, Line 716: if (!str_value.empty()) { Isn't this guaranteed by L697? http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@720 PS6, Line 720: case KuduColumnSchema::DataType::BOOL: : if (str_value == "true") { : *value = KuduValue::FromBool(true); : } else if (str_value == "false") { : *value = KuduValue::FromBool(false); Use boost::iequals() for the comparison to make it case-insensitive. http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@736 PS6, Line 736: case KuduColumnSchema::DECIMAL: Shouldn't this be DataType::DECIMAL? Same above for UNIXTIME_MICROS. http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@737 PS6, Line 737: return Status::NotSupported( : Substitute("$0 columns are not supported for setting default value by this tool", : KuduColumnSchema::DataTypeToString(type))); : default: : return Status::NotSupported(Substitute( : "Unsupported column type $0 for setting default value.", : KuduColumnSchema::DataTypeToString(type))); These can be combined. http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@750 PS6, Line 750: // Build a encoding_type_map from string to KuduColumnStorageAttributes::EncodingType. Same comment about static definition and an initializer_list. http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@759 PS6, Line 759: encoding_type_map.emplace( : "AUTO_ENCODING", KuduColumnStorageAttributes::EncodingType::AUTO_ENCODING); This is duplicated. http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@779 PS6, Line 779: // Build a compression_type_map to KuduColumnStorageAttributes::CompressionType. Same. http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@1021 PS6, Line 1021: .Description("Alter a column") Indentation here and below doesn't match that of the existing Actions. http://gerrit.cloudera.org:8080/#/c/13976/6/src/kudu/tools/tool_action_table.cc@1024 PS6, Line 1024: delete alter -- 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: 6 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: Tue, 13 Aug 2019 07:56:20 +0000 Gerrit-HasComments: Yes
