Grant Henke 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 2: (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: if (type == Type.CHAR) { : if (typeAttributes == null || !typeAttributes.hasLength() || typeAttributes.getLength() < CharUtil.MIN_CHAR_LENGTH : || typeAttributes.getLength() > CharUtil.MAX_CHAR_LENGTH) { : throw new IllegalArgumentException( : String.format("CHAR's length must be set and between %d and %d", CharUtil.MIN_CHAR_LENGTH, : CharUtil.MAX_CHAR_LENGTH)); : } : } : : if (type == Type.VARCHAR) { : if (typeAttributes == null || !typeAttributes.hasLength() || typeAttributes.getLength() < CharUtil.MIN_VARCHAR_LENGTH : || typeAttributes.getLength() > CharUtil.MAX_VARCHAR_LENGTH) { : throw new IllegalArgumentException( : String.format("VARCHAR's length must be set and between %d and %d", CharUtil.MIN_VARCHAR_LENGTH, : CharUtil.MAX_VARCHAR_LENGTH)); : } : } I think validation like this should be in the builders build() call and not the constructor. > forgot to set it in a test and took me a while to figure out what was wrong. Is the server not validating this and providing a clear error message back? http://gerrit.cloudera.org:8080/#/c/14050/2/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@398 PS2, Line 398: if (type != Type.DECIMAL && type != Type.CHAR Maybe use list.contains here now that there are 3 types to check and maybe more in the future. 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: * Add a CHAR for the specified column. Can you doc any truncation/padding details for all of these? Same in RowResult. http://gerrit.cloudera.org:8080/#/c/14050/2/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@788 PS2, Line 788: public String getChar(String columnName) { Should this be getUnpaddedChar? (here and in RowResult) 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: public void addChar(int columnIndex, byte[] val) { Why do we need byte[] versions of these? -- 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: 2 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 17:20:22 +0000 Gerrit-HasComments: Yes
