Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12890 )
Change subject: java/c++: ColumnSchema supports storing column comment ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/12890/2/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/2/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@401 PS2, Line 401: * Set the comment for this column. What does empty string mean? Should it be allowed? http://gerrit.cloudera.org:8080/#/c/12890/2/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java File java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java: http://gerrit.cloudera.org:8080/#/c/12890/2/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java@365 PS2, Line 365: * Change the comment for the column. Should doc what effect empty string has (it's for deleting an existing comment, right?). http://gerrit.cloudera.org:8080/#/c/12890/2/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: http://gerrit.cloudera.org:8080/#/c/12890/2/src/kudu/common/wire_protocol.cc@309 PS2, Line 309: attributes, type_attributes, std::move(comment)); Now that empty string means "no comment", I don't think we need boost::optional wrappers around comment fields at all: the empty string will tell us that there's no column comment. The only exception is in ColumnSchemaDelta, where we need explicit optionality in order to tell the difference between "user didn't ask us to modify the comment field" and "user asked us to delete the existing comment". -- 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: 2 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Tue, 02 Apr 2019 17:44:11 +0000 Gerrit-HasComments: Yes
