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 1: (6 comments) What happens if you try to add a comment to a column while creating a table in Impala today? Does table creation fail? Or is the comment added to the HMS table entry? Asking because I want to make sure there are no compatibility concerns with respect to Impala. 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 see this is the pattern used by the rest of this class, but I wonder why they don't just use boost::optional. Seems a lot easier. 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 figure out how to return "no comment". Should we make it the same as the empty string? 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. 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::optional to reflect that? 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 we can include it in with COMPARE_DEFAULTS (and change that to something more generic, like COMPARE_OTHER)? 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. -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Comment-Date: Tue, 26 Mar 2019 03:50:26 +0000 Gerrit-HasComments: Yes
