Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12890 )
Change subject: java: ColumnSchema supports storing column comment ...................................................................... Patch Set 1: (10 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() { > Maybe we could default to empty string instead of null. It might make users I think Lifu is mirroring a choice we made on the C++ side to treat "empty string" as a valid comment, which means "no comment" has to map to something else (like null). We can certainly revisit that choice as we haven't shipped anything yet, but we should make sure that whatever we do is consistent across both clients. 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 characters. 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(). 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 Well, we do we want to support _deleting_ a column, right? Wouldn't we model that as setting the comment back to null? 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. http://gerrit.cloudera.org:8080/#/c/12890/1/java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java File java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java: http://gerrit.cloudera.org:8080/#/c/12890/1/java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java@283 PS1, Line 283: requestBuilder.setSchema(ProtobufHelper.schemaToPb(schema, SchemaPBFlags.SCHEMA_PB_WITHOUT_COMMENT)); > Why can't the schema have a comment in this case? Is this just to save spac Yeah. A Write's schema ends up in the WAL, so there's no point in inflating it with the comments belonging to every column. Lifu did the same thing in the C++ client. A code comment explaining that would be good though. 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: - Don't bother setting up SCHEMA_PB_WITHOUT_NONE. - Don't bother assigning a value to each flag. - Pass flags around with an EnumSet. Use EnumSet.noneOf() to represent "serialize everything". 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 @Private already. 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? 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 in the C++ side too). -- 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:50:20 +0000 Gerrit-HasComments: Yes
