Dan Burkert has posted comments on this change.

Change subject: KUDU-1363: Add IN-list predicate type
......................................................................


Patch Set 17:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/2986/16//COMMIT_MSG
Commit Message:

PS16, Line 9: he C++ 
> 'C++ clients'  ?
Done


http://gerrit.cloudera.org:8080/#/c/2986/16/src/kudu/client/client.cc
File src/kudu/client/client.cc:

PS16, Line 726: char*
> nit: the file already states 'using std::vector', so this prefix might be d
Done


http://gerrit.cloudera.org:8080/#/c/2986/16/src/kudu/client/client.h
File src/kudu/client/client.h:

PS16, Line 848: p of the va
> nit: 'values container itself and its elements' ?
Done


http://gerrit.cloudera.org:8080/#/c/2986/16/src/kudu/common/column_predicate-test.cc
File src/kudu/common/column_predicate-test.cc:

Line 367:     top_list = {&values[1], &values[3], &values[6]};
> Consider also adding test cases for
Done


http://gerrit.cloudera.org:8080/#/c/2986/16/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:

Line 381:       };
> Does it make sense to check that other.values_ is not empty before proceedi
I've added a DCHECK, since it's assumed by this method.


Line 393:                                      other.values_.front(), 
search_by));
> Does it make sense to call SetToNone() before return?
Yes, great catch.  I've added a test that covers this as well.


Line 410:         return this->column_.type_info()->Compare(v, *other_start) != 
0;
> Before returning from this method, does it make sense to check and call Set
I've added a call to Simplify, which does the same thing.  Good catch.  One of 
the new tests you suggested covers this.


http://gerrit.cloudera.org:8080/#/c/2986/16/src/kudu/common/key_util.cc
File src/kudu/common/key_util.cc:

PS16, Line 179: 
> What if the IN list was empty?  Is that possible that it gets down to this 
We assume an IN list is never empty, but just to be safe i've added a DCHECK 
(since you have already pointed out a few places in this review where that 
could have been violated).


PS16, Line 242: that can be pushed.
> Ditto, but for the first element of the IN list.
Same; added DCHECK.


http://gerrit.cloudera.org:8080/#/c/2986/16/src/kudu/common/scan_spec-test.cc
File src/kudu/common/scan_spec-test.cc:

PS16, Line 74: const vector<T>&
> const vector<T>& ?
Done


http://gerrit.cloudera.org:8080/#/c/2986/16/src/kudu/common/scan_spec.cc
File src/kudu/common/scan_spec.cc:

PS16, Line 153: column).predicate_type();
> nit: it's not in the code you modified, but if you have some time, consider
Done


http://gerrit.cloudera.org:8080/#/c/2986/16/src/kudu/common/wire_protocol-test.cc
File src/kudu/common/wire_protocol-test.cc:

Line 339: TEST_F(WireProtocolTest, TestColumnPredicateInList) {
> Does it make sense to add a test to check for some "error" scenarios, like 
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar <abhyan...@gmail.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar <abhyan...@gmail.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to