Grant Henke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8882 )

Change subject: KUDU-721: [Java] Add DECIMAL column type support
......................................................................


Patch Set 14:

(16 comments)

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:       return this;
> Might be good to precondition this on the type being decimal.
Done


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:   @Override
> Use Objects.hash instead of hand-rolling the hash.  There's some allocation
Done


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:    */
> Specify the array must be zeroed.
Done


http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java@631
PS11, Line 631: teArray();
> can this use the 'bytes' array that was already extracted?  May need to swi
Done


http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java@656
PS11, Line 656:   }
> Looks like this would be more efficient as an in-place reverse, everywhere
Done


http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java@833
PS11, Line 833:   public static void setBigDecimal(final byte[] b, final 
BigDecimal n, int precision, final int offset) {
> It's worth copying in the impl here, since it's short and changing it later
I don't have access to the magnitude (mag) array for that quick check. Instead 
I can check the value fits via comparisons.


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.
Given it wasn't technically a completely new api but instead an overloaded 
setLowerBound and setUpperBound I though I should add this.


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(),
> Wrap this line.
Done


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.getTypeSize());
> Consider hoisting the size into a local variable now that it's no longer a
I thought about hoisting the sizes into a local variable that was set during 
init() but assumed getting the size might be a useful/common request. Instead I 
added a getTypeSize method to the ColumnSchema where the size is calculated at 
construction. This simplified getting the type size in a few places.


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.
Done


http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@1240
PS11, Line 1240:         BigDecimal max = DecimalUtil.maxValue(precision, 
scale);
> The logic here is wrong, it needs to be like the integer types above not th
Done


http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@1416
PS11, Line 1416:         BigDecimal smallestVal = 
DecimalUtil.smallestValue(scale);
> The !val.equals(maxVal) shouldn't be necessary here; it's only present in t
Done


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.
Done


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: trings.repeat("9", MAX_DECIMAL128_
> Perhaps cleaner as
Done


http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/util/DecimalUtil.java@146
PS11, Line 146: ColumnTypeAttrib
> This shouldn't need to be fully qualified.
Done


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.
Done



--
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: 14
Gerrit-Owner: Grant Henke <granthe...@gmail.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Grant Henke <granthe...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-Comment-Date: Tue, 13 Feb 2018 21:58:53 +0000
Gerrit-HasComments: Yes

Reply via email to