Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table ...................................................................... Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/14306/6/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/14306/6/src/kudu/client/schema.cc@665 PS6, Line 665: LOG(FATAL) << "Unhandled type " << type; > I think this place needs to define a invalid data type in the enum, such as Makes sense; that is a best practice we use elsewhere for protobuf enums. http://gerrit.cloudera.org:8080/#/c/14306/6/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/14306/6/src/kudu/tools/kudu-tool-test.cc@5433 PS6, Line 5433: "default_value": {"int_v": 1} > Here I define a new PB: The PB string type would work OK for most Kudu data types (ints, bool, float, double, string), but how would it work for binary? Should we require base64 encoding? You'd also need some special handling for decimal types, but I guess that would have been the case with ValuePB too. OK, I'm convinced: your suggestion is better, and we can make string work well enough. -- 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: 6 Gerrit-Owner: YangSong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong <[email protected]> Gerrit-Comment-Date: Tue, 08 Oct 2019 17:47:11 +0000 Gerrit-HasComments: Yes
