You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Will Berkeley (Code Review)" <ge...@cloudera.org> on 2016/12/09 17:36:14 UTC
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Will Berkeley has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5439
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
This patch exposes client APIs for the IS NOT NULL predicate and
adds an IS NULL predicate type.
These are both purely new features, so the changes are backwards-compatible.
Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.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/TestPartitionPruner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanPredicate.java
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/scan_token-test.cc
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol.cc
19 files changed, 591 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/5439/1
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5439/6/src/kudu/client/client.h
File src/kudu/client/client.h:
Line 941: KuduPredicate* MakePredicate(const Slice& col_name, const PredicateMakerFunc& f);
> As per the C++ ABI guidelines, I think it's safe to introduce this. But, co
Done. It was almost trivial. I just had to add the Schema as an extra parameter because of the reference to `data_` (http://stackoverflow.com/questions/1604853/) and the template function had to be defined in the header file (http://stackoverflow.com/questions/495021 maybe?), otherwise the function had "internal linkage". Thanks for taking a glance at it.
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged.
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
This patch exposes client APIs for the IS NOT NULL predicate and
adds an IS NULL predicate type.
These are both purely new features, so the changes are backwards-compatible.
Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Reviewed-on: http://gerrit.cloudera.org:8080/5439
Reviewed-by: Dan Burkert <da...@apache.org>
Tested-by: Kudu Jenkins
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.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/TestPartitionPruner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanPredicate.java
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/table-internal.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol.cc
22 files changed, 897 insertions(+), 109 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5439/4/src/kudu/client/client.cc
File src/kudu/client/client.cc:
Line 751: if (!col_schema.is_nullable()) {
Since you are already doing this check in ColumnPredicate::IsNull, could you skip it here? I think that would simplify this, since you shouldn't need the NonePredicateData class.
http://gerrit.cloudera.org:8080/#/c/5439/4/src/kudu/client/scan_token-internal.cc
File src/kudu/client/scan_token-internal.cc:
Line 197: if (predicate_pair.second.predicate_type() == PredicateType::None) {
Good catch - we were already implicitly short circuiting if there was a None predicate since the partition pruner would call CanShortCircuit() on the scan spec, and contain no partitions (e.g. no trips through the while loop below). However, I'm guessing adding this is necessary to avoid a CHECK failure when converting a None predicate to protobuf? In that case I think it would be better to do the check by calling
if (configuration_.CanShortCircuit()) {
return Status::OK();
}
immediately after the scan spec is optimized on line 171.
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/common/column_predicate.h
File src/kudu/common/column_predicate.h:
Line 184: return false;
> This is a little suspicious. I'm having a hard time following when CopyAnd
I didn't check the coverage here, but the "eval" vs "not eval" code path is related to whether the underlying encoding supports predicate evaluation. Agreed we should ensure coverage of it.
Will, can you double check that the new tests you've added covers both this and the below case? You may need to add randomization or parameterization over different encodings.
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5439
to look at the new patch set (#3).
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
This patch exposes client APIs for the IS NOT NULL predicate and
adds an IS NULL predicate type.
These are both purely new features, so the changes are backwards-compatible.
Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.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/TestPartitionPruner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanPredicate.java
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol.cc
21 files changed, 894 insertions(+), 102 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/5439/3
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Will Berkeley (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/5439
to look at the new patch set (#8).
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
This patch exposes client APIs for the IS NOT NULL predicate and
adds an IS NULL predicate type.
These are both purely new features, so the changes are backwards-compatible.
Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.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/TestPartitionPruner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanPredicate.java
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/table-internal.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol.cc
22 files changed, 897 insertions(+), 109 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/5439/8
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
Patch Set 2:
(12 comments)
Looks pretty good. Dan should probably take a look too.
http://gerrit.cloudera.org:8080/#/c/5439/2/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:
Line 568: case IS_NULL: return none(column);
does our 'in-list' API permit 'in (NULL, 1, 3) type queries? we should be careful of the semantics of those
http://gerrit.cloudera.org:8080/#/c/5439/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 467: }
add a test for IS NULL
http://gerrit.cloudera.org:8080/#/c/5439/2/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:
Line 518: // IS NOT NULL
extra line? or if you meant this to be a section header, separate and add a //------ or something
Line 567: // IS NULL
same
http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/client/client.cc
File src/kudu/client/client.cc:
PS2, Line 750: StringPiece name_sp(reinterpret_cast<const char*>(col_name.data()), col_name.size());
: const Schema* s = data_->schema_.schema_;
: int col_idx = s->find_column(name_sp);
: if (col_idx == Schema::kColumnNotFound) {
: // Since this function doesn't return an error, instead we create a special
: // predicate that just returns the errors when we add it to the scanner.
: //
: // This makes the API more "fluent".
: return new KuduPredicate(new ErrorPredicateData(
: Status::NotFound("column not found", col_name)));
: }
this chunk of code is now showing up a bunch of places. Can you extract a utility function something like:
namespace {
template<class PredicateMakerFunc>
KuduPredicate* MakePredicate(const Slice& col_name,
const PredicateMakerFunc& f) {
// do the stuff to resolve int col_idx and return an error
// if necessary
return PredicateMakerFunc(col_idx);
}
}
... NewIsNotNullPredicate(...) {
return MakePredicate(col_name, [&](int col_idx) {
return new KuduPredicate(new IsNotNullPredicateData(s->column(col_idx));
});
}
http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
Line 198: { // in list predicate
is the addition of this test coverage related to this patch? perhaps add separately?
Line 213: // However, pruning's not perfect yet.
can you add a TODO(KUDU-xxx) with an appropriate JIRA about what is missing here?
Line 214: //ASSERT_GE(3, tokens.size());
the above seems to indicate that pruning is working _better_ than we expected?
Line 344: { // in list predicate
same as above
Line 366: { // IS NOT NULL predicate
can you add a case with IS NOT NULL on a non-nullable column (eg the PK?) just to make sure it properly returns no tokens?
http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:
Line 562: for (size_t i = 0; i < block.nrows(); i++) {
can you copy the above TODO?
http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:
PS2, Line 465: ColumnPredicate p = ColumnPredicate::IsNull(col);
: *predicate = ColumnPredicate::IsNull(col);
hrm, extra line here? (also fix above)
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
Patch Set 2:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/5439/2/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:
Line 487: return new KuduPredicate(PredicateType.IS_NULL, column, null, null);
Is it correct to check here if the column is non-nullable, and if so return a NONE predicate?
http://gerrit.cloudera.org:8080/#/c/5439/2/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:
Line 518: // IS NOT NULL
> extra line? or if you meant this to be a section header, separate and add a
Yah I didn't do a great job of making the section headers stand out when I wrote this, adding some dashes to all of them would help.
Line 552: // [------->
Please add the other two range types (upper bounded range, upper + lower bounded range) to IS NOT NULL and IS NULL.
Line 659: // [--)
Do you mind fixing this ascii while you are making changes here?
Line 667: // [--)
same here
Line 937: public void testToString() {
add IS NULL test here
http://gerrit.cloudera.org:8080/#/c/5439/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java:
Line 630: // timestamp IS NOT NULL
Add an IS NULL case.
http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/client/client.cc
File src/kudu/client/client.cc:
Line 764: KuduPredicate* KuduTable::NewIsNullPredicate(const Slice& col_name) {
Same comment here about IS NULL on non-nullable columns.
http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:
PS2, Line 805: CountRows(table, {}) - 1
I think this can be simplified to values.size() - 1 here and elsewhere.
http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
Line 214: //ASSERT_GE(3, tokens.size());
> the above seems to indicate that pruning is working _better_ than we expect
Perhaps this confusion is because https://github.com/apache/kudu/commit/ab76440e9c2447ba1c572b957a958e90e9e9b87b landed only recently?
Line 219: { // IS NOT NULL predicate
Could also use an IS NULL case here and below.
http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/common/column_predicate-test.cc
File src/kudu/common/column_predicate-test.cc:
Line 681: // [-------)
Add [---> and <-----)
http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:
Line 149: None(move(column));
OK cool - looks like you are doing this simplification. Could you add the equivalent on the Java side and add specific tests where appropriate?
PS2, Line 669: // Fallthrough intended.
either add this to the None case or just remove it. Not sure what our style guide says, but I think it's pretty clear when there isn't a body.
http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/common/column_predicate.h
File src/kudu/common/column_predicate.h:
Line 184: return false;
This is a little suspicious. I'm having a hard time following when CopyAndEval is and is not used in cfile_reader.cc. Does this codepath at least have good coverage? Todd, did you check this out?
PS2, Line 257:
attach the & to the type
PS2, Line 260: e
same
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5439/7/src/kudu/client/client.cc
File src/kudu/client/client.cc:
PS7, Line 720: const Schema
> Here and below should be 'const Schema&', otherwise it's making an implicit
Done
http://gerrit.cloudera.org:8080/#/c/5439/7/src/kudu/client/table-internal.h
File src/kudu/client/table-internal.h:
PS7, Line 42: const Schema& schema,
: const PredicateMakerFunc& f)
> These two should be lined up with the one above, so the 'const's are in a r
Done
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Will Berkeley (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/5439
to look at the new patch set (#7).
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
This patch exposes client APIs for the IS NOT NULL predicate and
adds an IS NULL predicate type.
These are both purely new features, so the changes are backwards-compatible.
Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.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/TestPartitionPruner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanPredicate.java
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/table-internal.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol.cc
22 files changed, 897 insertions(+), 109 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/5439/7
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
Patch Set 3:
(26 comments)
New tests parameterized over column encodings probably needs some cleanup, but the core code should be good. I ran the new encoding tests in slow mode as well.
http://gerrit.cloudera.org:8080/#/c/5439/2/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:
Line 487: if (!column.isNullable()) {
> Is it correct to check here if the column is non-nullable, and if so return
Done
Line 568: switch (type) {
> It does not, since it would make it more complicated, and adding a NULL val
Resolved by Dan's comment. Thanks Dan.
http://gerrit.cloudera.org:8080/#/c/5439/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 467: ).size());
> add a test for IS NULL
Done
http://gerrit.cloudera.org:8080/#/c/5439/2/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:
Line 518: // =
> Yah I didn't do a great job of making the section headers stand out when I
Done for all sections.
Line 552: // IS NOT NULL
> Please add the other two range types (upper bounded range, upper + lower bo
Done
Line 567: // IS NOT NULL AND
> same
Done
Line 659: // IS NULL AND
> Do you mind fixing this ascii while you are making changes here?
Done
Line 667:
> same here
Done
Line 937: KuduPredicate.none(byteCol));
> add IS NULL test here
Done
http://gerrit.cloudera.org:8080/#/c/5439/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java:
Line 630: // timestamp IS NOT NULL
> Add an IS NULL case.
Done
http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/client/client.cc
File src/kudu/client/client.cc:
PS2, Line 750: return MakePredicate(col_name, [&](const ColumnSchema& col_schema) {
: if (!col_schema.is_nullable()) {
: return new KuduPredicate(new NonePredicateData(col_schema));
: }
: return new KuduPredicate(new IsNullPredicateData(col_schema));
: });
: }
:
: ////////////////////////////////////////////////////////////
: // Error
: ///
> this chunk of code is now showing up a bunch of places. Can you extract a u
Done
Line 764: }
> Same comment here about IS NULL on non-nullable columns.
Done
http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:
PS2, Line 805: CountRows(table, { table
> I think this can be simplified to values.size() - 1 here and elsewhere.
values.size(), actually. Done.
http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
Line 198: { // IS NOT NULL predicate
> is the addition of this test coverage related to this patch? perhaps add se
Moving to new changelist.
Line 213: ElementDeleter deleter(&tokens);
> can you add a TODO(KUDU-xxx) with an appropriate JIRA about what is missing
I think this comment will be stale now.
Line 214: KuduScanTokenBuilder builder(table.get());
> Perhaps this confusion is because https://github.com/apache/kudu/commit/ab7
(new changelist for this)
Line 219: ASSERT_GE(0, tokens.size());
> Could also use an IS NULL case here and below.
Done
Line 344: ASSERT_EQ(6, tokens.size());
> same as above
Done
Line 366: unique_ptr<KuduPartialRow> upper_bound(schema.NewRow());
> can you add a case with IS NOT NULL on a non-nullable column (eg the PK?) j
Done
http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/common/column_predicate-test.cc
File src/kudu/common/column_predicate-test.cc:
Line 681: // [-------)
> Add [---> and <-----)
Done
http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:
Line 149: None(move(column));
> OK cool - looks like you are doing this simplification. Could you add the
Done
Line 562: // TODO: make this more efficient by using bitwise operations on the
> can you copy the above TODO?
Done
PS2, Line 669:
> either add this to the None case or just remove it. Not sure what our styl
Done
http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/common/column_predicate.h
File src/kudu/common/column_predicate.h:
PS2, Line 257: &
> attach the & to the type
Done
PS2, Line 260: e&
> same
Done
http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:
PS2, Line 465: break;
: }
> hrm, extra line here? (also fix above)
Done
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Will Berkeley (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/5439
to look at the new patch set (#6).
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
This patch exposes client APIs for the IS NOT NULL predicate and
adds an IS NULL predicate type.
These are both purely new features, so the changes are backwards-compatible.
Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.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/TestPartitionPruner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanPredicate.java
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol.cc
21 files changed, 888 insertions(+), 103 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/5439/6
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
Patch Set 1:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/5439/1/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:
Line 236: void CheckIntPredicates(shared_ptr<KuduTable> table) {
> warning: the parameter 'table' is copied for each invocation but only used
Done
http://gerrit.cloudera.org:8080/#/c/5439/1/src/kudu/client/scan_predicate-internal.h
File src/kudu/client/scan_predicate-internal.h:
Line 122: IsNotNullPredicateData(ColumnSchema col)
> warning: single-argument constructors must be marked explicit to avoid unin
Done
Line 123: : col_(move(col)) {
> error: use of undeclared identifier 'move' [clang-diagnostic-error]
Done
Line 123: : col_(move(col)) {
> error: use of undeclared identifier 'move' [clang-diagnostic-error]
Done
Line 144: IsNullPredicateData(ColumnSchema col)
> warning: single-argument constructors must be marked explicit to avoid unin
Done
Line 145: : col_(move(col)) {
> error: use of undeclared identifier 'move' [clang-diagnostic-error]
Done
Line 145: : col_(move(col)) {
> error: use of undeclared identifier 'move' [clang-diagnostic-error]
Done
http://gerrit.cloudera.org:8080/#/c/5439/1/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
Line 215: ASSERT_EQ(3, CountRows(std::move(tokens)));
> warning: passing result of std::move() as a const reference argument; no mo
Done
Line 228: ASSERT_EQ(200, CountRows(std::move(tokens)));
> warning: passing result of std::move() as a const reference argument; no mo
Done
Line 362: ASSERT_EQ(4, CountRows(std::move(tokens)));
> warning: passing result of std::move() as a const reference argument; no mo
Done
Line 375: ASSERT_EQ(300, CountRows(std::move(tokens)));
> warning: passing result of std::move() as a const reference argument; no mo
Done
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5439/6/src/kudu/client/client.h
File src/kudu/client/client.h:
Line 941: KuduPredicate* MakePredicate(const Slice& col_name, const PredicateMakerFunc& f);
As per the C++ ABI guidelines, I think it's safe to introduce this. But, couldn't it be moved to KuduTable::Data instead, which would help client.h stay public API only? I took a quick look at how MakePredicate is used and I think it could be moved, though I'm not 100% sure.
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5439
to look at the new patch set (#4).
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
This patch exposes client APIs for the IS NOT NULL predicate and
adds an IS NULL predicate type.
These are both purely new features, so the changes are backwards-compatible.
Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.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/TestPartitionPruner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanPredicate.java
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol.cc
21 files changed, 901 insertions(+), 103 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/5439/4
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5439
to look at the new patch set (#2).
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
This patch exposes client APIs for the IS NOT NULL predicate and
adds an IS NULL predicate type.
These are both purely new features, so the changes are backwards-compatible.
Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.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/TestPartitionPruner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanPredicate.java
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/scan_token-test.cc
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol.cc
19 files changed, 592 insertions(+), 74 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/5439/2
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5439
to look at the new patch set (#5).
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
This patch exposes client APIs for the IS NOT NULL predicate and
adds an IS NULL predicate type.
These are both purely new features, so the changes are backwards-compatible.
Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.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/TestPartitionPruner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanPredicate.java
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol.cc
21 files changed, 888 insertions(+), 103 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/5439/5
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5439/7/src/kudu/client/client.cc
File src/kudu/client/client.cc:
PS7, Line 720: const Schema
Here and below should be 'const Schema&', otherwise it's making an implicit copy.
http://gerrit.cloudera.org:8080/#/c/5439/7/src/kudu/client/table-internal.h
File src/kudu/client/table-internal.h:
PS7, Line 42: const Schema& schema,
: const PredicateMakerFunc& f)
These two should be lined up with the one above, so the 'const's are in a row.
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5439/4/src/kudu/client/client.cc
File src/kudu/client/client.cc:
Line 751: if (!col_schema.is_nullable()) {
> Since you are already doing this check in ColumnPredicate::IsNull, could yo
Done
http://gerrit.cloudera.org:8080/#/c/5439/4/src/kudu/client/scan_token-internal.cc
File src/kudu/client/scan_token-internal.cc:
Line 197: if (predicate_pair.second.predicate_type() == PredicateType::None) {
> Good catch - we were already implicitly short circuiting if there was a Non
Done
http://gerrit.cloudera.org:8080/#/c/5439/4/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:
Line 562: // TODO: make this more efficient by using bitwise operations on the
> warning: missing username/bug in TODO [google-readability-todo]
Done
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No
[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5439/2/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:
Line 568: case IS_NULL: return none(column);
> does our 'in-list' API permit 'in (NULL, 1, 3) type queries? we should be c
It does not, since it would make it more complicated, and adding a NULL value to an IN list has no effect, since NULL is not equal to NULL. For example:
postgres=# create table t (a bigint primary key, b bigint);
CREATE TABLE
postgres=# insert into t values (0, 0), (1, null);
INSERT 0 2
postgres=# select * from t where b in (99, null);
a | b
---+---
(0 rows)
--
To view, visit http://gerrit.cloudera.org:8080/5439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes