Grant Henke 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) Thank you for all the work on this helifu. 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 should be okay. Especially because empty is the default. 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." I don't think we need to view "empty comment" as s special deleting operation. Because we default to "", the only thing you need to worry about is "setting" a comment. Whether it's setting it to "some new string" or setting it to "", then end result is the user set a string for the comment. 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. I don't think we need to prevent users from "setting" a column to an empty string. 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. 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 empty, we can still display it that way. It may also be confusing if a user sets a column comment of "-" themselves. -- 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 <hzhel...@corp.netease.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu <hzhel...@corp.netease.com> Gerrit-Comment-Date: Wed, 03 Apr 2019 13:33:47 +0000 Gerrit-HasComments: Yes
