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 9: (5 comments) 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@698 PS6, Line 698: case KuduColumnSchema::DataType::INT8: > If users want to set default value of a STRING/BINARY column to an empty st Just to clarify: with the new JSON encoding, empty strings are treated as valid default values just like any other string, right? Could you verify that with a unit test? http://gerrit.cloudera.org:8080/#/c/13976/9/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/13976/9/src/kudu/tools/tool_action_table.cc@173 PS9, Line 173: const unordered_map<string, KuduColumnStorageAttributes::CompressionType> kCompressionTypeMap { We try to avoid declaring global non-POD-type variables. They're not so much an issue in executables, but in libraries they affect loading performance and their initialization order is undefined (which is a problem when one global depends on another). Instead, declare these as static variables within whichever function needs them. http://gerrit.cloudera.org:8080/#/c/13976/9/src/kudu/tools/tool_action_table.cc@706 PS9, Line 706: Substitute("unable to parse value for column type $0", : KuduColumnSchema::DataTypeToString(type)) Maybe you can declare this up front so you needn't duplicate it in multiple case statements. http://gerrit.cloudera.org:8080/#/c/13976/9/src/kudu/tools/tool_action_table.cc@767 PS9, Line 767: KuduColumnSchema col_schema = schema.Column(0); Could you add a comment here explaining that we're not actually interested in the first column of the schema, but that there's no default constructor for KuduColumnSchema so we're forced to create one using some valid column? http://gerrit.cloudera.org:8080/#/c/13976/9/src/kudu/tools/tool_action_table.cc@797 PS9, Line 797: auto it = kCompressionTypeMap.find(compression_type_uc); : if (it == kCompressionTypeMap.end()) { Should be able to use one of the FindOr* functions from gutil/map-util.h here (and in ColumnSetEncoding). -- 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: 9 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 16:50:56 +0000 Gerrit-HasComments: Yes
