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/08/16 18:46:46 UTC

[kudu-CR] Fix ScanToken generation with non-covering range partitions 

Hello Adar Dembo, Will Berkeley, Todd Lipcon,

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

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

to review the following change.

Change subject: Fix ScanToken generation with non-covering range partitions
......................................................................

Fix ScanToken generation with non-covering range partitions

The scan token builder was missing some critical code to handle non-covering
range partitioned tables correctly. This fix is mostly copy/paste from
scanner-internal, which is largely doing the same thing.

Change-Id: Iff3ec3e2399b191c71595c96212471b1e21c7446
---
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
2 files changed, 128 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iff3ec3e2399b191c71595c96212471b1e21c7446
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] [c++-client] fix KuduScanTokenBuilder token generation bugs

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

Change subject: [c++-client] fix KuduScanTokenBuilder token generation bugs
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4007/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:

Line 341:   private int countScanTokenRows (List<KuduScanToken> tokens) throws Exception {
Nit: countScanTokenRows(


http://gerrit.cloudera.org:8080/#/c/4007/2/src/kudu/integration-tests/flex_partitioning-itest.cc
File src/kudu/integration-tests/flex_partitioning-itest.cc:

Line 268:   void CheckScanWithColumnPredicate(Slice col_name, int lower, int upper);
Should probably mention that this Check() variant calls the new one.


Line 367:   LOG(WARNING) << "Checking scan tokens: " << col_name << " [" << lower << ", " << upper << "]";
Just for debugging?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iff3ec3e2399b191c71595c96212471b1e21c7446
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] [c++-client] fix KuduScanTokenBuilder token generation bugs

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

Change subject: [c++-client] fix KuduScanTokenBuilder token generation bugs
......................................................................


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iff3ec3e2399b191c71595c96212471b1e21c7446
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] [c++-client] fix KuduScanTokenBuilder token generation bugs

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

Change subject: [c++-client] fix KuduScanTokenBuilder token generation bugs
......................................................................


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/2953/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iff3ec3e2399b191c71595c96212471b1e21c7446
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] Fix ScanToken generation with non-covering range partitions

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

Change subject: Fix ScanToken generation with non-covering range partitions
......................................................................


Patch Set 1: Code-Review+2

Would be nice to consolidate that code, but I don't see an easy way to do it.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iff3ec3e2399b191c71595c96212471b1e21c7446
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] Fix ScanToken generation with non-covering range partitions

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

Change subject: Fix ScanToken generation with non-covering range partitions
......................................................................


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/2950/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iff3ec3e2399b191c71595c96212471b1e21c7446
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] [c++-client] fix KuduScanTokenBuilder token generation bugs

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

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

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

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

Change subject: [c++-client] fix KuduScanTokenBuilder token generation bugs
......................................................................

[c++-client] fix KuduScanTokenBuilder token generation bugs

This commit fixes two critical issues in the KuduScanTokenBuilder
implementation:

  1. Column predicates are now correctly carried through to the scan token.
     Prior testing didn't catch this because the predicates were being
     transformed into PK bounds, which have always worked correctly. This is
     only an issue on the serialization side, so it doesn't affect scan tokens
     generated by the Java client and deserialized by the C++ client.
  2. Token building now works on tables with non-covering range partitioned
     tables. The fix is mostly copy/paste from scanner-internal, which is
     very similar.

flex_partitioning-itest has been extended to check scan tokens, and the scan
token unit tests have been updated. I also added a unit test for issue 1 on the
Java side to add some confidence that the Java side is not affected.

Change-Id: Iff3ec3e2399b191c71595c96212471b1e21c7446
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/integration-tests/flex_partitioning-itest.cc
4 files changed, 265 insertions(+), 37 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iff3ec3e2399b191c71595c96212471b1e21c7446
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] [c++-client] fix KuduScanTokenBuilder token generation bugs

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

Change subject: [c++-client] fix KuduScanTokenBuilder token generation bugs
......................................................................


Patch Set 3:

Build Started http://104.196.14.100/job/kudu-gerrit/2955/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iff3ec3e2399b191c71595c96212471b1e21c7446
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] [c++-client] fix KuduScanTokenBuilder token generation bugs

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

Change subject: [c++-client] fix KuduScanTokenBuilder token generation bugs
......................................................................


[c++-client] fix KuduScanTokenBuilder token generation bugs

This commit fixes two critical issues in the KuduScanTokenBuilder
implementation:

  1. Column predicates are now correctly carried through to the scan token.
     Prior testing didn't catch this because the predicates were being
     transformed into PK bounds, which have always worked correctly. This is
     only an issue on the serialization side, so it doesn't affect scan tokens
     generated by the Java client and deserialized by the C++ client.
  2. Token building now works on tables with non-covering range partitioned
     tables. The fix is mostly copy/paste from scanner-internal, which is
     very similar.

flex_partitioning-itest has been extended to check scan tokens, and the scan
token unit tests have been updated. I also added a unit test for issue 1 on the
Java side to add some confidence that the Java side is not affected.

Change-Id: Iff3ec3e2399b191c71595c96212471b1e21c7446
Reviewed-on: http://gerrit.cloudera.org:8080/4007
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/integration-tests/flex_partitioning-itest.cc
4 files changed, 265 insertions(+), 37 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Iff3ec3e2399b191c71595c96212471b1e21c7446
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] [c++-client] fix KuduScanTokenBuilder token generation bugs

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

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

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

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

Change subject: [c++-client] fix KuduScanTokenBuilder token generation bugs
......................................................................

[c++-client] fix KuduScanTokenBuilder token generation bugs

This commit fixes two critical issues in the KuduScanTokenBuilder
implementation:

  1. Column predicates are now correctly carried through to scan token. Prior
     testing didn't catch this because the predicates were being transformed
     into PK bounds, which have always worked correctly. This is only an issue
     on the serialization side, so it doesn't affect scan tokens generated by
     the Java client and deserialized by the C++ client.
  2. Token building now works on tables with non-covering range partitioned
     tables. The fix is mostly copy/paste from scanner-internal, which is
     very similar.

flex_partitioning-itest has been extended to check scan tokens, and the scan
token unit tests have been updated. I also added a unit test for issue 1 on the
Java side to add some confidence that the Java side is not affected.

Change-Id: Iff3ec3e2399b191c71595c96212471b1e21c7446
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/integration-tests/flex_partitioning-itest.cc
4 files changed, 267 insertions(+), 37 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iff3ec3e2399b191c71595c96212471b1e21c7446
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>