helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12849 )
Change subject: KUDU-1711: ColumnSchema supports storing column comment ...................................................................... Patch Set 3: (9 comments) yes, the impala catalog uses kudu java client, so i will try to add comment api to it, though i am not familiar with java. http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema-internal.h File src/kudu/client/schema-internal.h: http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema-internal.h@117 PS1, Line 117: > Thanks. If you're interested, perhaps you could convert the other has_foo b all right, i will do it in a next patch. http://gerrit.cloudera.org:8080/#/c/12849/3/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/12849/3/src/kudu/client/schema.h@246 PS3, Line 246: /// @note "no comment" and "empty comment" are the same, : /// so both of them return empty string. > Nit: reword as "Both columns with no comments and empty comments will retur Done http://gerrit.cloudera.org:8080/#/c/12849/3/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/12849/3/src/kudu/client/schema.cc@625 PS3, Line 625: std:: > Don't need std:: prefix here. Done http://gerrit.cloudera.org:8080/#/c/12849/3/src/kudu/client/schema.cc@679 PS3, Line 679: string > Maybe return a const ref instead? It is "boost::optional<std::string> comment_" in ColumnSchema, so we cannot use const ref here. http://gerrit.cloudera.org:8080/#/c/12849/3/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/12849/3/src/kudu/common/schema.h@187 PS3, Line 187: // name: column name > Add 'comment' to the list of parameters here. Done http://gerrit.cloudera.org:8080/#/c/12849/3/src/kudu/common/schema.h@255 PS3, Line 255: std::string comment() const { : return comment_ ? *comment_ : ""; : } > Could we just return the comment wrapped in boost::optional? Schema is an i Done http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/common/schema.h@315 PS1, Line 315: > Can you also rename COMPARE_DEFAULTS, since now it includes the comment fie Done http://gerrit.cloudera.org:8080/#/c/12849/3/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: http://gerrit.cloudera.org:8080/#/c/12849/3/src/kudu/common/wire_protocol.cc@306 PS3, Line 306: attributes, type_attributes, comment); > Can std::move(comment) here. Sure, it should be added. http://gerrit.cloudera.org:8080/#/c/12849/3/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/12849/3/src/kudu/master/catalog_manager.cc@1331 PS3, Line 1331: Status ValidateCommentIdentifier(const string& id) { > Looks like this could share even more code with ValidateIdentifier; you can Done -- To view, visit http://gerrit.cloudera.org:8080/12849 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82d3228100c262cc4a5c63eaf696927c88bc8990 Gerrit-Change-Number: 12849 Gerrit-PatchSet: 3 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Wed, 27 Mar 2019 06:16:58 +0000 Gerrit-HasComments: Yes
