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 2: (7 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: PS2: > Some new lines are too long; please wrap. Done 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::type That's where I wanted to do it first, but that would allow not setting it and it should always be set for CHAR/VARCHAR types - forgot to set it in a test and took me a while to figure out what was wrong. 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 us Done 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, addV Done 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? Done 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 s Done 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 checkU Done -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 13 Aug 2019 08:50:13 +0000 Gerrit-HasComments: Yes
