Dan Burkert has posted comments on this change.
Change subject: KUDU-1363: Add in-list predicates for extracting a set of
equalities. Adds support for clients to provide a set of equalities for a given
column. The following types of in-list predicates are supported: integer,
float, double, binary, string. Eg: column I
......................................................................
Patch Set 1:
(8 comments)
Hey Sameer,
Thanks for the patch! I made a few nitpicks, mostly about style. I think the
backend of the implementation is looking pretty good, but I'm not sure about
the direction you took with the public API. I think it would be easier and
simpler to add a new method to KuduTable:
KuduPredicate* KuduTable::NewInListPredicate(const Slice& col_name,
std::vector<KuduValue*> value);
I think this would remove the need for most of the changes to KuduValue. I'm
going to add Todd and Adar to the review, since I'm sure they have some
thoughts on this as well.
http://gerrit.cloudera.org:8080/#/c/2986/1/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:
Line 25: #include <set>
please sort within the other system includes.
Line 40: using std::set;
please sort within the using block.
http://gerrit.cloudera.org:8080/#/c/2986/1/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:
Line 47: : predicate_type_(predicate_type),
please don't use tabs here and elsewhere.
Line 494: case PredicateType::InList: return 3;
I think In should be above Range here.
http://gerrit.cloudera.org:8080/#/c/2986/1/src/kudu/common/column_predicate.h
File src/kudu/common/column_predicate.h:
Line 23: #include <sstream>
is this used anywhere?
Line 37: typedef vector<void*> List_values;
a matter of opinion, but I don't think this typedef is aiding comprehensibility.
Line 111: // The values ae not copied, and must outlive the returned
predicate.
s/ae/are
http://gerrit.cloudera.org:8080/#/c/2986/1/src/kudu/common/wire_protocol-test.cc
File src/kudu/common/wire_protocol-test.cc:
Line 18: #include <vector>
sort within system includes.
--
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: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar <[email protected]>
Gerrit-Reviewer: Dan Burkert <[email protected]>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
