helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12890 )
Change subject: java: ColumnSchema supports storing column comment ...................................................................... Patch Set 1: (14 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 Grant is proposing: Sure. Thanks, Adar :) http://gerrit.cloudera.org:8080/#/c/12890/1/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@244 PS1, Line 244: if (comment != null) { > If we default to "" this check wont be needed. Done 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: PS1: > Please check the wrapping on the modified lines; some are over 100 characte Done http://gerrit.cloudera.org:8080/#/c/12890/1/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java@296 PS1, Line 296: pb.setSchema(ProtobufHelper.schemaToPb(lowerBound.getSchema(), SchemaPBFlags.SCHEMA_PB_WITHOUT_COMMENT)); > Why can't the schema have a comment in this case? The comment is useless for adding range partition. http://gerrit.cloudera.org:8080/#/c/12890/1/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java@355 PS1, Line 355: pb.setSchema(ProtobufHelper.schemaToPb(lowerBound.getSchema(), SchemaPBFlags.SCHEMA_PB_WITHOUT_COMMENT)); > Why can't the schema have a comment in this case? Likewise, the comment is useless for dropping range partition. http://gerrit.cloudera.org:8080/#/c/12890/1/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java@367 PS1, Line 367: public AlterTableOptions ChangeComment(String name, String comment) { > Nit: should be named changeComment(). Done 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) { > Since setting a null comment isn't allowed, it makes more sense to default Done http://gerrit.cloudera.org:8080/#/c/12890/1/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java@375 PS1, Line 375: AlterTableRequestPB.AlterColumn.newBuilder(); > There are some tabs here and on L377; please convert to spaces. Done http://gerrit.cloudera.org:8080/#/c/12890/1/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java File java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java: http://gerrit.cloudera.org:8080/#/c/12890/1/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@45 PS1, Line 45: * The flags that are not included while serializing. > I think you can simplify this a bit: Done http://gerrit.cloudera.org:8080/#/c/12890/1/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@47 PS1, Line 47: @InterfaceAudience.Public : @InterfaceStability.Evolving > I don't think you need these given that it's in a class that's marked @Priv Done http://gerrit.cloudera.org:8080/#/c/12890/1/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@49 PS1, Line 49: public enum SchemaPBFlags { > Maybe SchemaPBConversionFlags would be more clear? Done http://gerrit.cloudera.org:8080/#/c/12890/1/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@150 PS1, Line 150: String comment = pb.hasComment() ? pb.getComment() : null; > default empty string "". Done 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. > I really think setting to an empty string should be okay and null comments Done 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 Done -- 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-Reviewer: helifu <hzhel...@corp.netease.com> Gerrit-Comment-Date: Tue, 02 Apr 2019 10:21:25 +0000 Gerrit-HasComments: Yes