Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/14050 )
Change subject: KUDU-1938 [java] Add support for CHAR/VARCHAR pt 4 ...................................................................... Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/14050/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/14050/2/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@121 PS2, Line 121: this.typeSize = type.getSize(typeAttributes); : this.wireType = wireType; : this.comment = comment; : } : : /** : * Get the column's Type : * @return the type : */ : public Type getType() { : return type; : } : : /** : * Get the column's name : * @return A string representation of the name : */ > I think validation like this should be in the builders build() call and not > the constructor. that makes sense, moved them to build() > Is the server not validating this and providing a clear error message back? Nope, I didn't write server-side validations. Should I? http://gerrit.cloudera.org:8080/#/c/14050/2/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@398 PS2, Line 398: */ > Maybe use list.contains here now that there are 3 types to check and maybe Done http://gerrit.cloudera.org:8080/#/c/14050/2/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java File java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java: http://gerrit.cloudera.org:8080/#/c/14050/2/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@642 PS2, Line 642: * > Can you doc any truncation/padding details for all of these? Same in RowRes Added them. RowResult doesn't do any truncation/padding, should I point out it's unpadded? http://gerrit.cloudera.org:8080/#/c/14050/2/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@788 PS2, Line 788: */ > Should this be getUnpaddedChar? (here and in RowResult) Done http://gerrit.cloudera.org:8080/#/c/14050/4/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java File java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java: http://gerrit.cloudera.org:8080/#/c/14050/4/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@692 PS4, Line 692: * @param val value to add > Why do we need byte[] versions of these? KeyEncoder uses them. I still need to convert these byte[]s to Strings for length check to be able to do the string manipulation though, so I guess it would make sense to do the conversion in KeyEncoder. Should I rewrite it that way and drop the (int, byte[]) and (String, byte[]) variants? -- To view, visit http://gerrit.cloudera.org:8080/14050 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I03edf5e65409e895512d5cd81a607180632e8995 Gerrit-Change-Number: 14050 Gerrit-PatchSet: 5 Gerrit-Owner: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 13 Aug 2019 21:35:08 +0000 Gerrit-HasComments: Yes
