Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8882 )
Change subject: KUDU-721: [Java] Add DECIMAL column type support ...................................................................... Patch Set 11: (17 comments) Similar concerns from the C++ side, I'm not seeing any coverage of PK decimals with predicates. http://gerrit.cloudera.org:8080/#/c/8882/11/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/8882/11/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@331 PS11, Line 331: this.typeAttributes = typeAttributes; Might be good to precondition this on the type being decimal. http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/ColumnTypeAttributes.java File java/kudu-client/src/main/java/org/apache/kudu/ColumnTypeAttributes.java: http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/ColumnTypeAttributes.java@98 PS11, Line 98: int result = hasPrecision ? 1 : 0; Use Objects.hash instead of hand-rolling the hash. There's some allocation overhead, but hard coded primes are pretty yucky, and I don't see a high-perf usecase for putting these in a hashmap. http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java File java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java: http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java@618 PS11, Line 618: * @param b The array to write to. Specify the array must be zeroed. http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java@631 PS11, Line 631: n.toByteArray() can this use the 'bytes' array that was already extracted? May need to switch reverseBytes to be in-place. http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java@656 PS11, Line 656: private static byte[] reverseBytes(final byte[] b) { Looks like this would be more efficient as an in-place reverse, everywhere that calls it has already made defensive copies. http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java@833 PS11, Line 833: // TODO: use n.unscaledValue().intValueExact() when we drop Java7 support. It's worth copying in the impl here, since it's short and changing it later could be considered backwards incompatible. https://github.com/openjdk-mirror/jdk/blob/jdk8u/jdk8u/master/src/share/classes/java/math/BigInteger.java?utf8=%E2%9C%93#L4398-L4403 http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java File java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java: http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java@41 PS11, Line 41: @Deprecated This class is deprecated, so you shouldn't add any new APIs to it. http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java: http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java@1062 PS11, Line 1062: return Bytes.getDecimal(value, typeAttributes.getPrecision(), typeAttributes.getScale()).toString(); Wrap this line. http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java File java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java: http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java@306 PS11, Line 306: rows.put(rowData, currentRowOffset, col.getType().getSize(col.getTypeAttributes())); Consider hoisting the size into a local variable now that it's no longer a cheap accessor. This method is called once for each row in a write batch, so I think it's perf sensitive. http://gerrit.cloudera.org:8080/#/c/8882/11/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/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@529 PS11, Line 529: * Get the specified column's Decimal add a period. http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@1240 PS11, Line 1240: if (existing.equals(incremented)) { The logic here is wrong, it needs to be like the integer types above not the float types. Please make sure there is coverage of this in a decimal primary key predicate test. http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@1416 PS11, Line 1416: return !val.equals(maxVal) && The !val.equals(maxVal) shouldn't be necessary here; it's only present in the integer cases to prevent overflow. http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java File java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java: http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@331 PS11, Line 331: * Get the specified column's Decimal Add a period. http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/util/DecimalUtil.java File java/kudu-client/src/main/java/org/apache/kudu/util/DecimalUtil.java: http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/util/DecimalUtil.java@44 PS11, Line 44: 9999999999999999999999999999999999 Perhaps cleaner as new BigInteger(Strings.repeat("9", MAX_DECIMAL128_PRECISION)) http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/util/DecimalUtil.java@146 PS11, Line 146: org.apache.kudu. This shouldn't need to be fully qualified. http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/test/java/org/apache/kudu/client/TestBytes.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestBytes.java: http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/test/java/org/apache/kudu/client/TestBytes.java@103 PS11, Line 103: // DECIMAL (32 bits) Add negative test cases. http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java: http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java@68 PS11, Line 68: private static final ColumnSchema decimalCol = Add one for each size bucket. -- To view, visit http://gerrit.cloudera.org:8080/8882 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6240e3cfe0d6328b68c50099d442ffeeab6c9fd9 Gerrit-Change-Number: 8882 Gerrit-PatchSet: 11 Gerrit-Owner: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 07 Feb 2018 20:17:19 +0000 Gerrit-HasComments: Yes
