Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17851 )
Change subject: [tool] kudu tool, support add column ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/17851/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17851/2//COMMIT_MSG@7 PS2, Line 7: kudu tool, support add column CLI to add a column http://gerrit.cloudera.org:8080/#/c/17851/2/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/17851/2/src/kudu/client/schema.h@310 PS2, Line 310: /// @todo maybe should united as one, such as kudu::EncodingType(pb) and : /// kudu::client::KuduColumnStorageAttributes::EncodingType, kudu::CompressionType(pb) : /// and kudu::client::KuduColumnStorageAttributes::CompressionType : /// @return Storage attributes of the column I don't understand this comment. http://gerrit.cloudera.org:8080/#/c/17851/2/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/17851/2/src/kudu/tools/tool_action_table.cc@128 PS2, Line 128: "AUTO_ENCODING, PLAIN_ENCODING, PREFIX_ENCODING, RLE, DICT_ENCODING, " : "BIT_SHUFFLE, GROUP_VARINT"); Type of encoding for the column including AUTO_ENCODING, PLAIN_ENCODING... http://gerrit.cloudera.org:8080/#/c/17851/2/src/kudu/tools/tool_action_table.cc@131 PS2, Line 131: "DEFAULT_COMPRESSION, NO_COMPRESSION, SNAPPY, LZ4, ZLIB" Type of compression for the column; http://gerrit.cloudera.org:8080/#/c/17851/2/src/kudu/tools/tool_action_table.cc@985 PS2, Line 985: const string& data_type_name = FindOrDie(context.required_args, kDataTypeArg); : KuduColumnSchema::DataType data_type; : RETURN_NOT_OK(KuduColumnSchema::StringToDataType(data_type_name, &data_type)); This required param should go at the top after column_name. http://gerrit.cloudera.org:8080/#/c/17851/2/src/kudu/tools/tool_action_table.cc@984 PS2, Line 984: { : const string& data_type_name = FindOrDie(context.required_args, kDataTypeArg); : KuduColumnSchema::DataType data_type; : RETURN_NOT_OK(KuduColumnSchema::StringToDataType(data_type_name, &data_type)); : column_spec->Type(data_type); : : if (!FLAGS_default_value.empty()) { : KuduValue* value = nullptr; : RETURN_NOT_OK(ParseValueOfType(FLAGS_default_value, data_type, &value)); : column_spec->Default(value); : } : } Nit: Don't see a need for the block scope and below as well. -- To view, visit http://gerrit.cloudera.org:8080/17851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I93e6c2af07e9fa5aa909b4a09b79b7c54401d4e3 Gerrit-Change-Number: 17851 Gerrit-PatchSet: 2 Gerrit-Owner: Anonymous Coward <shenxingwuy...@gmail.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 29 Sep 2021 23:16:14 +0000 Gerrit-HasComments: Yes
