helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12849 )
Change subject: KUDU-1711: ColumnSchema supports storing column comment ...................................................................... Patch Set 1: (10 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 http://gerrit.cloudera.org:8080/#/c/12849/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12849/1//COMMIT_MSG@9 PS1, Line 9: This patch intends to support storing column comment in ColumnSchema, > One question here (I could probably figure this out but I'm lazy so I'll ju Ah, that's a good point! I did make that mistake. Thank you, Todd. Now, two areas have been reinforced: 1) WriteRequestPB from client; 2) AlterTableRequestPB from client; 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: bool has_comment; > I think we had some fear of using boost, which might be incorrect Done http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema-internal.h@117 PS1, Line 117: bool has_comment; > I see this is the pattern used by the rest of this class, but I wonder why Done http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema.h@245 PS1, Line 245: const std::string& comment() const; > Since we can't wrap with boost::optional here (client header), we need to f both "no comment" and "empty comment" return empty string. http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema.h@245 PS1, Line 245: const std::string& comment() const; > I think it's reasonable enough to say that "no comment" and empty string ar yep, i think so too. http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema.cc@684 PS1, Line 684: const std::string& KuduColumnSchema::comment() const { > Can drop std:: prefix here. 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@208 PS1, Line 208: const std::string& comment = "") > If the comment is supposed to be optional, perhaps we can wrap it in boost: Done http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/common/schema.h@315 PS1, Line 315: COMPARE_COMMENT = 1 << 3, > Do we actually need this broken out into its own comparison mode? Perhaps w Done http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/common/wire_protocol.cc@299 PS1, Line 299: std::string comment = ""; > Would rather represent "unset" as a non-existent boost::optional. Done http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/master/catalog_manager.cc@1369 PS1, Line 1369: if (col.comment().length() > FLAGS_max_column_comment_length) { > we should probably validate that the column is valid UTF8 as well (we can r 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: 1 Gerrit-Owner: helifu <hzhel...@corp.netease.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: helifu <hzhel...@corp.netease.com> Gerrit-Comment-Date: Tue, 26 Mar 2019 11:06:16 +0000 Gerrit-HasComments: Yes
