[kudu-CR] java: ColumnSchema supports storing column comment

["helifu (Code Review)" <ge...@cloudera.org>]

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 <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Tue, 02 Apr 2019 10:21:25 +0000
Gerrit-HasComments: Yes