You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2016/11/03 18:18:49 UTC

[kudu-CR] Add KuduTable.getFormattedRangePartitions method 

Hello Jean-Daniel Cryans, Matthew Jacobs,

I'd like you to do a code review.  Please visit

    http://gerrit.cloudera.org:8080/4934

to review the following change.

Change subject: Add KuduTable.getFormattedRangePartitions method
......................................................................

Add KuduTable.getFormattedRangePartitions method

This adds an Impala-specific, unstable API to the KuduTable class for
retrieving the list of the range partitions in a table. The partitions
are formatted according to the proposed Impala DDL syntax for creating
tables.

Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.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/Partition.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/StringUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
A java/kudu-client/src/test/java/org/apache/kudu/util/TestStringUtil.java
8 files changed, 627 insertions(+), 51 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/34/4934/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4934
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>

[kudu-CR] Add KuduTable.getFormattedRangePartitions method

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.

Change subject: Add KuduTable.getFormattedRangePartitions method
......................................................................


Patch Set 2:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/4934/2/java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java:

PS2, Line 271: ByteBuffer buf = ByteBuffer.wrap(key);
unused variable


Line 274: 
nit: extra blank line


http://gerrit.cloudera.org:8080/#/c/4934/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java:

Line 223:       if (!Iterators.all(partition.getHashBuckets().iterator(), Predicates.equalTo(0))) continue;
> nit: I personally prefer if statements to not be on a single line, especial
+1


http://gerrit.cloudera.org:8080/#/c/4934/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:

PS2, Line 804:  + 1
no +1 here for equals check


PS2, Line 820: // Check that b is 1 byte longer than a, the extra byte is 0, and all other bytes are equal.
Stale/copy-paste comment


http://gerrit.cloudera.org:8080/#/c/4934/2/java/kudu-client/src/main/java/org/apache/kudu/client/Partition.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Partition.java:

PS2, Line 187: Pretty prints
nit: Technically this returns a string as opposed to actually printing something; maybe change to something like "Returns a pretty-printed string describing this tablet partition's range component"


Line 190:    */
nit: add an @return


-- 
To view, visit http://gerrit.cloudera.org:8080/4934
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add KuduTable.getFormattedRangePartitions method

Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.

Change subject: Add KuduTable.getFormattedRangePartitions method
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4934/2/java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java:

PS2, Line 268: public
> Yes it's used in Partition.formatRangePartition
Then it can be package-private?


-- 
To view, visit http://gerrit.cloudera.org:8080/4934
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add KuduTable.getFormattedRangePartitions method

Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.

Change subject: Add KuduTable.getFormattedRangePartitions method
......................................................................


Patch Set 4: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/4934
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] Add KuduTable.getFormattedRangePartitions method

Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.

Change subject: Add KuduTable.getFormattedRangePartitions method
......................................................................


Patch Set 3: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/4934
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] Add KuduTable.getFormattedRangePartitions method

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Jean-Daniel Cryans, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4934

to look at the new patch set (#4).

Change subject: Add KuduTable.getFormattedRangePartitions method
......................................................................

Add KuduTable.getFormattedRangePartitions method

This adds an Impala-specific, unstable API to the KuduTable class for
retrieving the list of the range partitions in a table. The partitions
are formatted according to the proposed Impala DDL syntax for creating
tables.

Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.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/Partition.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/StringUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestOperation.java
A java/kudu-client/src/test/java/org/apache/kudu/util/TestStringUtil.java
9 files changed, 690 insertions(+), 58 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/34/4934/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4934
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add KuduTable.getFormattedRangePartitions method

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: Add KuduTable.getFormattedRangePartitions method
......................................................................


Patch Set 2:

(1 comment)

I think as long as we upgrade in step with Impala, we shouldn't have any backwards compat concerns.  This is completely client-side, so we won't need to support it forever.

http://gerrit.cloudera.org:8080/#/c/4934/2/java/kudu-client/src/main/java/org/apache/kudu/util/StringUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/StringUtil.java:

PS2, Line 27: org.apache.hadoop.hive.ql.parse.BaseSemanticAnalyzer.escapeSQLString
> Can you use a {@link} here so that it's possible to navigate to it in an ID
At least in IntelliJ this doesn't work because Hive isn't a dependency of the project.  Does it work in Eclipse?  If so I can add it.


-- 
To view, visit http://gerrit.cloudera.org:8080/4934
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[kudu-CR] Add KuduTable.getFormattedRangePartitions method

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: Add KuduTable.getFormattedRangePartitions method
......................................................................


Patch Set 2:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/4934/2/java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java:

PS2, Line 268: public
> Does this need to be public?
Yes it's used in Partition.formatRangePartition


PS2, Line 271: ByteBuffer buf = ByteBuffer.wrap(key);
> unused variable
Done


Line 274: 
> nit: extra blank line
Done


http://gerrit.cloudera.org:8080/#/c/4934/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java:

PS2, Line 208: the tables
> nit: this table's
Done


Line 223:       if (!Iterators.all(partition.getHashBuckets().iterator(), Predicates.equalTo(0))) continue;
> nit: I personally prefer if statements to not be on a single line, especial
Done


http://gerrit.cloudera.org:8080/#/c/4934/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:

PS2, Line 579: appendCellValueDebugString
> nit: might as well put that in the enum itself.
All of these methods depend heavily on the internal representation of the PartialRow, I don't think it should be exposed outside of this class.


PS2, Line 785: Returns {@code true} if the cell values for the given column are equal in the two rows.
> nit: that text should be in @return. Write something here that's like "Chec
Done


PS2, Line 804:  + 1
> no +1 here for equals check
Done


PS2, Line 820: // Check that b is 1 byte longer than a, the extra byte is 0, and all other bytes are equal.
> Stale/copy-paste comment
Done


PS2, Line 839: Returns
> Same comment as above.
Done


http://gerrit.cloudera.org:8080/#/c/4934/2/java/kudu-client/src/main/java/org/apache/kudu/client/Partition.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Partition.java:

PS2, Line 187: Pretty prints
> nit: Technically this returns a string as opposed to actually printing some
Done


Line 190:    */
> nit: add an @return
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4934
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add KuduTable.getFormattedRangePartitions method

Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.

Change subject: Add KuduTable.getFormattedRangePartitions method
......................................................................


Patch Set 2:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/4934/2/java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java:

PS2, Line 268: public
Does this need to be public?


http://gerrit.cloudera.org:8080/#/c/4934/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java:

PS2, Line 208: the tables
nit: this table's


Line 223:       if (!Iterators.all(partition.getHashBuckets().iterator(), Predicates.equalTo(0))) continue;
nit: I personally prefer if statements to not be on a single line, especially if sandwiched like this, for better readability.


http://gerrit.cloudera.org:8080/#/c/4934/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:

PS2, Line 579: appendCellValueDebugString
nit: might as well put that in the enum itself.


PS2, Line 785: Returns {@code true} if the cell values for the given column are equal in the two rows.
nit: that text should be in @return. Write something here that's like "Checks if the specified cell is equal in both rows" or something similar.


PS2, Line 802: switch (type) {
Guess you could move that into the enum as well.


PS2, Line 839: Returns
Same comment as above.


Line 858:     switch (type) {
Same comment as above.


-- 
To view, visit http://gerrit.cloudera.org:8080/4934
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[kudu-CR] Add KuduTable.getFormattedRangePartitions method

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4934

to look at the new patch set (#2).

Change subject: Add KuduTable.getFormattedRangePartitions method
......................................................................

Add KuduTable.getFormattedRangePartitions method

This adds an Impala-specific, unstable API to the KuduTable class for
retrieving the list of the range partitions in a table. The partitions
are formatted according to the proposed Impala DDL syntax for creating
tables.

Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.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/Partition.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/StringUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestOperation.java
A java/kudu-client/src/test/java/org/apache/kudu/util/TestStringUtil.java
9 files changed, 628 insertions(+), 52 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/34/4934/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4934
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>

[kudu-CR] Add KuduTable.getFormattedRangePartitions method

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: Add KuduTable.getFormattedRangePartitions method
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4934/2/java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java:

PS2, Line 268: public
> Then it can be package-private?
The entire class is package private.


-- 
To view, visit http://gerrit.cloudera.org:8080/4934
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add KuduTable.getFormattedRangePartitions method

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Add KuduTable.getFormattedRangePartitions method
......................................................................


Patch Set 2:

(1 comment)

Just passing through; not a real review.

Logistically, what needs to happen in order for us to change this API without breaking compatibility? Or do we not care about compatibility here because Impala's Kudu dependency is versioned and well managed?

http://gerrit.cloudera.org:8080/#/c/4934/2/java/kudu-client/src/main/java/org/apache/kudu/util/StringUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/StringUtil.java:

PS2, Line 27: org.apache.hadoop.hive.ql.parse.BaseSemanticAnalyzer.escapeSQLString
Can you use a {@link} here so that it's possible to navigate to it in an IDE?


-- 
To view, visit http://gerrit.cloudera.org:8080/4934
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[kudu-CR] Add KuduTable.getFormattedRangePartitions method

Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.

Change subject: Add KuduTable.getFormattedRangePartitions method
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4934/2/java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java:

PS2, Line 268: public
> The entire class is package private.
Ugh you're right. I'm not a fan of marking publicly reachable methods public, but I guess this class is already doing this a lot.


-- 
To view, visit http://gerrit.cloudera.org:8080/4934
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add KuduTable.getFormattedRangePartitions method

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4934

to look at the new patch set (#3).

Change subject: Add KuduTable.getFormattedRangePartitions method
......................................................................

Add KuduTable.getFormattedRangePartitions method

This adds an Impala-specific, unstable API to the KuduTable class for
retrieving the list of the range partitions in a table. The partitions
are formatted according to the proposed Impala DDL syntax for creating
tables.

Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.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/Partition.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/StringUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestOperation.java
A java/kudu-client/src/test/java/org/apache/kudu/util/TestStringUtil.java
9 files changed, 631 insertions(+), 52 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/34/4934/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4934
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add KuduTable.getFormattedRangePartitions method

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.

Change subject: Add KuduTable.getFormattedRangePartitions method
......................................................................


Add KuduTable.getFormattedRangePartitions method

This adds an Impala-specific, unstable API to the KuduTable class for
retrieving the list of the range partitions in a table. The partitions
are formatted according to the proposed Impala DDL syntax for creating
tables.

Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
Reviewed-on: http://gerrit.cloudera.org:8080/4934
Reviewed-by: Jean-Daniel Cryans <jd...@apache.org>
Tested-by: Kudu Jenkins
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.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/Partition.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/StringUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestOperation.java
A java/kudu-client/src/test/java/org/apache/kudu/util/TestStringUtil.java
9 files changed, 690 insertions(+), 58 deletions(-)

Approvals:
  Jean-Daniel Cryans: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/4934
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ia9b263d2444314d46533191918833840e75b7ba7
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>