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 4: (6 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 n hmm, I can't remember if we do anything special for Decimal. I thought the attributes were validated server side. Maybe not. 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. > Added them. RowResult doesn't do any truncation/padding, should I point out I think with the name change it's clear enough. 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) { > KeyEncoder uses them. I still need to convert these byte[]s to Strings for If KeyEncoder can use the string version I think that makes sense to convert to a string there. It's a quick oneline wrapper. So adding the API here seems overkill. If you decide to keep these, if they only exist for KeyEncoder can they be package-private and not a part of the public api? http://gerrit.cloudera.org:8080/#/c/14050/7/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/7/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@762 PS7, Line 762: public String getString(String columnName) { Is it technically correct to substring by characters while the string is still UTF-16 (java default)? Or should we convert to bytes and then truncate the extra bytes? http://gerrit.cloudera.org:8080/#/c/14050/7/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@764 PS7, Line 764: } What happens in the case where a CHAR column has trailing spaces to so that length is >= val.length? Should we move this above that if statement? http://gerrit.cloudera.org:8080/#/c/14050/7/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@769 PS7, Line 769: * @return a string This else statement is the same as the end the if statement. Can this be re-worked to reuse the end logic? -- 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: 4 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: Thu, 15 Aug 2019 16:53:49 +0000 Gerrit-HasComments: Yes
