Grant Henke has posted comments on this change. ( )

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

Patch Set 14:

File java/kudu-client/src/main/java/org/apache/kudu/
PS11, Line 331:       return this;
> Might be good to precondition this on the type being decimal.
File java/kudu-client/src/main/java/org/apache/kudu/
PS11, Line 98:   @Override
> Use Objects.hash instead of hand-rolling the hash.  There's some allocation
File java/kudu-client/src/main/java/org/apache/kudu/client/
PS11, Line 618:    */
> Specify the array must be zeroed.
PS11, Line 631: teArray();
> can this use the 'bytes' array that was already extracted?  May need to swi
PS11, Line 656:   }
> Looks like this would be more efficient as an in-place reverse, everywhere
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.
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.
File java/kudu-client/src/main/java/org/apache/kudu/client/
PS11, Line 1062:        return Bytes.getDecimal(value, 
> Wrap this line.
File java/kudu-client/src/main/java/org/apache/kudu/client/
PS11, Line 306:             rows.put(rowData, currentRowOffset, 
> 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.
File java/kudu-client/src/main/java/org/apache/kudu/client/
PS11, Line 529:    * Get the specified column's Decimal.
> add a period.
PS11, Line 1240:         BigDecimal max = DecimalUtil.maxValue(precision, 
> The logic here is wrong, it needs to be like the integer types above not th
PS11, Line 1416:         BigDecimal smallestVal = 
> The !val.equals(maxVal) shouldn't be necessary here; it's only present in t
File java/kudu-client/src/main/java/org/apache/kudu/client/
PS11, Line 331:    * Get the specified column's Decimal.
> Add a period.
File java/kudu-client/src/main/java/org/apache/kudu/util/
PS11, Line 44: trings.repeat("9", MAX_DECIMAL128_
> Perhaps cleaner as
PS11, Line 146: ColumnTypeAttrib
> This shouldn't need to be fully qualified.
File java/kudu-client/src/test/java/org/apache/kudu/client/
PS11, Line 103:     // DECIMAL (32 bits)
> Add negative test cases.

To view, visit
To unsubscribe, visit

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6240e3cfe0d6328b68c50099d442ffeeab6c9fd9
Gerrit-Change-Number: 8882
Gerrit-PatchSet: 14
Gerrit-Owner: Grant Henke <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Grant Henke <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-Comment-Date: Tue, 13 Feb 2018 21:58:53 +0000
Gerrit-HasComments: Yes

Reply via email to