Adar Dembo 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: (7 comments) I didn't review the test changes. 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: PS2: Some new lines are too long; please wrap. 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)); : } : } Curious why you're verifying this here and not in ColumnSchemaBuilder::typeAttributes()? 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@45 PS2, Line 45: import static java.nio.charset.StandardCharsets.UTF_8; : import static org.apache.kudu.Type.*; Wildcard imports are convenient, but they can obscure the actual imports used, which is why we generally only use them in tests. http://gerrit.cloudera.org:8080/#/c/14050/2/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@692 PS2, Line 692: public void addChar(int columnIndex, byte[] val) { Before these four methods, the order was addChar, addVarchar, addChar, addVarchar. Here it's addChar, addChar, addVarchar, addVarchar. Could you pick one and use it consistently? http://gerrit.cloudera.org:8080/#/c/14050/2/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@726 PS2, Line 726: UTF_8 Shouldn't this also be CharsetUtil.UTF_8? http://gerrit.cloudera.org:8080/#/c/14050/2/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@737 PS2, Line 737: substring = substring.replaceAll(" +$", ""); This compiles a new Pattern with every call; perhaps we should statically store a single compiled Pattern? Alternatively, you can avoid the construction of an extra Java String if you combine the truncation and whitespace removal into one backwards loop (for CHAR). http://gerrit.cloudera.org:8080/#/c/14050/2/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@739 PS2, Line 739: byte[] bytes = Bytes.fromString(substring); : checkUtf8(bytes); : addVarLengthData(columnIndex, bytes); : } else { : byte[] bytes = Bytes.fromString(val); : checkUtf8(bytes); : addVarLengthData(columnIndex, bytes); : } Can this be combined across the two cases? Will let you do away with checkUtf8 too. -- 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 06:50:02 +0000 Gerrit-HasComments: Yes
