Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table ...................................................................... Patch Set 17: (3 comments) http://gerrit.cloudera.org:8080/#/c/14306/17/src/kudu/client/schema.cc File src/kudu/client/schema.cc: PS17: You can remove the std:: prefixes in the new code. http://gerrit.cloudera.org:8080/#/c/14306/16/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/14306/16/src/kudu/tools/tool.proto@342 PS16, Line 342: // The encoding type of column. Default type is "AUTO_ENCODING", the types : // "PLAIN_ENCODING", "PREFIX_ENCODING", "GROUP_VARINT", "RLE", : // "DICT_ENCODING", "BIT_SHUFFLE" are also supported. : optional string encoding = 6; : // The compression of column. Default type is "DEFAULT_COMPRESSION", the : // types "NO_COMPRESSION", "SNAPPY", "LZ4", "ZLIB" are also supported. : optional string compression = 7; > If using PB enums, it maybe looks user-unfriendly. We talked about this in https://gerrit.cloudera.org/c/14306/3/src/kudu/tools/tool.proto#346. For now let's start with true enums where possible; if it turns out to be too confusing, we can change it later. http://gerrit.cloudera.org:8080/#/c/14306/17/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/14306/17/src/kudu/tools/tool_action_table.cc@1010 PS17, Line 1010: KuduSchema kudu_schema; Nit: move this definition to just before ParseTableSchema. The rule to follow here is to narrow the scope of a local variable as much as possible: this makes the code a little bit more readable (i.e. less state to track in your head). -- 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: 17 Gerrit-Owner: YangSong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong <[email protected]> Gerrit-Comment-Date: Wed, 16 Oct 2019 03:43:08 +0000 Gerrit-HasComments: Yes
