You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2017/12/19 19:56:40 UTC
[kudu-CR] KUDU-721: [Java] Add DECIMAL column type support
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8882
Change subject: KUDU-721: [Java] Add DECIMAL column type support
......................................................................
KUDU-721: [Java] Add DECIMAL column type support
This patch adds basic support to the Java client to
create, read, and write tables with DECIMAL columns.
Change-Id: I6240e3cfe0d6328b68c50099d442ffeeab6c9fd9
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
A java/kudu-client/src/main/java/org/apache/kudu/ColumnTypeAttributes.java
M java/kudu-client/src/main/java/org/apache/kudu/Schema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
A java/kudu-client/src/main/java/org/apache/kudu/util/DecimalUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestBytes.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRowResult.java
18 files changed, 964 insertions(+), 40 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/8882/1
--
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: newchange
Gerrit-Change-Id: I6240e3cfe0d6328b68c50099d442ffeeab6c9fd9
Gerrit-Change-Number: 8882
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@gmail.com>
[kudu-CR] KUDU-721: [Java] Add DECIMAL column type support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8882/14/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/14/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@1240
PS14, Line 1240: BigDecimal max = DecimalUtil.maxValue(precision, scale);
> This is a little bit different than the c++ IncrementCell which is based on
I have a follow up work item to update the c++ side to take the type attributes in account which would align these two. I can open a jira to track that.
--
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 <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 13 Feb 2018 22:19:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: [Java] Add DECIMAL column type support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 13 Feb 2018 21:58:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: [Java] Add DECIMAL column type support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8882
to look at the new patch set (#9).
Change subject: KUDU-721: [Java] Add DECIMAL column type support
......................................................................
KUDU-721: [Java] Add DECIMAL column type support
This patch adds basic support to the Java client to
create, read, and write tables with DECIMAL columns.
Change-Id: I6240e3cfe0d6328b68c50099d442ffeeab6c9fd9
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
A java/kudu-client/src/main/java/org/apache/kudu/ColumnTypeAttributes.java
M java/kudu-client/src/main/java/org/apache/kudu/Schema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
A java/kudu-client/src/main/java/org/apache/kudu/util/DecimalUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestBytes.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRowResult.java
22 files changed, 1,120 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/8882/9
--
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: newpatchset
Gerrit-Change-Id: I6240e3cfe0d6328b68c50099d442ffeeab6c9fd9
Gerrit-Change-Number: 8882
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: [Java] Add DECIMAL column type support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8882
to look at the new patch set (#5).
Change subject: KUDU-721: [Java] Add DECIMAL column type support
......................................................................
KUDU-721: [Java] Add DECIMAL column type support
This patch adds basic support to the Java client to
create, read, and write tables with DECIMAL columns.
Change-Id: I6240e3cfe0d6328b68c50099d442ffeeab6c9fd9
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
A java/kudu-client/src/main/java/org/apache/kudu/ColumnTypeAttributes.java
M java/kudu-client/src/main/java/org/apache/kudu/Schema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
A java/kudu-client/src/main/java/org/apache/kudu/util/DecimalUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestBytes.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRowResult.java
20 files changed, 1,059 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/8882/5
--
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: newpatchset
Gerrit-Change-Id: I6240e3cfe0d6328b68c50099d442ffeeab6c9fd9
Gerrit-Change-Number: 8882
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-721: [Java] Add DECIMAL column type support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8882 )
Change subject: KUDU-721: [Java] Add DECIMAL column type support
......................................................................
KUDU-721: [Java] Add DECIMAL column type support
This patch adds basic support to the Java client to
create, read, and write tables with DECIMAL columns.
Change-Id: I6240e3cfe0d6328b68c50099d442ffeeab6c9fd9
Reviewed-on: http://gerrit.cloudera.org:8080/8882
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@cloudera.com>
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
A java/kudu-client/src/main/java/org/apache/kudu/ColumnTypeAttributes.java
M java/kudu-client/src/main/java/org/apache/kudu/Schema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
A java/kudu-client/src/main/java/org/apache/kudu/util/DecimalUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestBytes.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKeyEncoding.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRowResult.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanPredicate.java
24 files changed, 1,502 insertions(+), 62 deletions(-)
Approvals:
Kudu Jenkins: Verified
Dan Burkert: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: I6240e3cfe0d6328b68c50099d442ffeeab6c9fd9
Gerrit-Change-Number: 8882
Gerrit-PatchSet: 15
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: [Java] Add DECIMAL column type support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8882
to look at the new patch set (#12).
Change subject: KUDU-721: [Java] Add DECIMAL column type support
......................................................................
KUDU-721: [Java] Add DECIMAL column type support
This patch adds basic support to the Java client to
create, read, and write tables with DECIMAL columns.
Note: Also fixes a small incrementColumn bug for float
and double columns found when testing.
Change-Id: I6240e3cfe0d6328b68c50099d442ffeeab6c9fd9
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
A java/kudu-client/src/main/java/org/apache/kudu/ColumnTypeAttributes.java
M java/kudu-client/src/main/java/org/apache/kudu/Schema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
A java/kudu-client/src/main/java/org/apache/kudu/util/DecimalUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestBytes.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKeyEncoding.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRowResult.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanPredicate.java
24 files changed, 1,504 insertions(+), 64 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/8882/12
--
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: newpatchset
Gerrit-Change-Id: I6240e3cfe0d6328b68c50099d442ffeeab6c9fd9
Gerrit-Change-Number: 8882
Gerrit-PatchSet: 12
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: [Java] Add DECIMAL column type support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8882
to look at the new patch set (#4).
Change subject: KUDU-721: [Java] Add DECIMAL column type support
......................................................................
KUDU-721: [Java] Add DECIMAL column type support
This patch adds basic support to the Java client to
create, read, and write tables with DECIMAL columns.
Change-Id: I6240e3cfe0d6328b68c50099d442ffeeab6c9fd9
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
A java/kudu-client/src/main/java/org/apache/kudu/ColumnTypeAttributes.java
M java/kudu-client/src/main/java/org/apache/kudu/Schema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
A java/kudu-client/src/main/java/org/apache/kudu/util/DecimalUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestBytes.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRowResult.java
M src/kudu/integration-tests/decimal-itest.cc
19 files changed, 968 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/8882/4
--
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: newpatchset
Gerrit-Change-Id: I6240e3cfe0d6328b68c50099d442ffeeab6c9fd9
Gerrit-Change-Number: 8882
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-721: [Java] Add DECIMAL column type support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8882
to look at the new patch set (#13).
Change subject: KUDU-721: [Java] Add DECIMAL column type support
......................................................................
KUDU-721: [Java] Add DECIMAL column type support
This patch adds basic support to the Java client to
create, read, and write tables with DECIMAL columns.
Change-Id: I6240e3cfe0d6328b68c50099d442ffeeab6c9fd9
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
A java/kudu-client/src/main/java/org/apache/kudu/ColumnTypeAttributes.java
M java/kudu-client/src/main/java/org/apache/kudu/Schema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
A java/kudu-client/src/main/java/org/apache/kudu/util/DecimalUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestBytes.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKeyEncoding.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRowResult.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanPredicate.java
24 files changed, 1,502 insertions(+), 62 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/8882/13
--
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: newpatchset
Gerrit-Change-Id: I6240e3cfe0d6328b68c50099d442ffeeab6c9fd9
Gerrit-Change-Number: 8882
Gerrit-PatchSet: 13
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-721: [Java] Add DECIMAL column type support
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 07 Feb 2018 20:17:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: [Java] Add DECIMAL column type support
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 14: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8882/14/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/14/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@1240
PS14, Line 1240: BigDecimal max = DecimalUtil.maxValue(precision, scale);
This is a little bit different than the c++ IncrementCell which is based on the physical type. I don't think this will cause problems, but it's something to keep in mind.
--
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 <gr...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 13 Feb 2018 22:11:51 +0000
Gerrit-HasComments: Yes