helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12890 )
Change subject: java/c++: ColumnSchema supports storing column comment ...................................................................... Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/12890/6/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java File java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java: http://gerrit.cloudera.org:8080/#/c/12890/6/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@408 PS6, Line 408: throw new IllegalArgumentException("The comment should not be empty"); > I don't think we need this check. A user can set an empty comment and it sh Done http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/client/schema.h@445 PS6, Line 445: /// The comment for the column. An empty comment means deleting an > I think you can keep the docs as simple as "The comment for the column." Done http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/client/schema.cc@314 PS6, Line 314: return Status::InvalidArgument("cannot delete comment during CreateTable", > See my other comments on ColumnSchema.java and schema.h. Done http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/common/schema.h@347 PS6, Line 347: // "empty comment" means "no comment". > nit: I don't think you need this doc line here. Done http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/server/webui_util.cc File src/kudu/server/webui_util.cc: http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/server/webui_util.cc@53 PS6, Line 53: col_json["comment"] = !col.comment().empty() ? col.comment() : "-"; > I don't think we need the empty check here. In the case the comment is empt Done -- To view, visit http://gerrit.cloudera.org:8080/12890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c Gerrit-Change-Number: 12890 Gerrit-PatchSet: 6 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Thu, 04 Apr 2019 02:48:49 +0000 Gerrit-HasComments: Yes
