Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12890 )
Change subject: java: ColumnSchema supports storing column comment ...................................................................... Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/12890/1/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/1/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@213 PS1, Line 213: public String getComment() { > I think Lifu is mirroring a choice we made on the C++ side to treat "empty Apologies I didn't review that earlier patch. For me, null really needs to have a good purpose to be used. Otherwise it leads to bugs/exceptions for little good reason. What would the value of having a null column comment vs and empty string be? http://gerrit.cloudera.org:8080/#/c/12890/1/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/1/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java@368 PS1, Line 368: if (comment == null) { > Well, we do we want to support _deleting_ a column, right? Wouldn't we mode Sure, if that is the decisions. I was just noting how this "comment cannot be null" check backs up my claim about null columns comments. http://gerrit.cloudera.org:8080/#/c/12890/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java: http://gerrit.cloudera.org:8080/#/c/12890/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java@749 PS1, Line 749: // Change the comments to null. > It should be possible to "delete" a comment too; we should test that (and i I really think setting to an empty string should be okay and null comments don't need to be a thing. -- 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: 1 Gerrit-Owner: helifu <hzhel...@corp.netease.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 29 Mar 2019 21:55:22 +0000 Gerrit-HasComments: Yes
