Adar Dembo 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) > @adar,the table will be created successfully while adding a comment to a > column in Impala, but the comment will be lost. and here is a jira about > column comments for kudu tables: > https://issues.apache.org/jira/browse/IMPALA-5351 Thanks. When you merge this, please update the Impala ticket to let them know that column comments have been added. http://gerrit.cloudera.org:8080/#/c/12849/2/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: http://gerrit.cloudera.org:8080/#/c/12849/2/src/kudu/client/batcher.cc@a79 PS2, Line 79: > My local IWYU complained that this line should be removed but remote IWYU n Yeah, it's frustrating that IWYU offers different suggestions depending on the version of libstdc++ that it's running against. 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: > Done Thanks. If you're interested, perhaps you could convert the other has_foo booleans here into boost::optional, in a follow-up 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 return empty strings here." 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. 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. 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 internal class so it's OK to use boost::optional here. 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: > Done Can you also rename COMPARE_DEFAULTS, since now it includes the comment field, which is neither a read nor a write default? 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. 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 change ValidateUTF8 to receive a maximum length as an argument, then pass either FLAGS_max_identifier_length or FLAGS_max_column_length down. -- 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: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Tue, 26 Mar 2019 18:53:56 +0000 Gerrit-HasComments: Yes
