Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15122 )
Change subject: [client] KUDU-2483 Add IN Bloom filter predicate to C++ client ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/15122/3/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/15122/3/src/kudu/client/client.h@1114 PS3, Line 1114: KuduValue* lower_bound_inclusive = NULL, //NOLINT(modernize-use-nullptr) : KuduValue* upper_bound_exclusive = NULL); //NOLINT(modernize-use-nullptr) > Nit: // NOLINT (space between slashes and NOLINT). Done http://gerrit.cloudera.org:8080/#/c/15122/1/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/15122/1/src/kudu/client/client.h@1104 PS1, Line 1104: /// must correspond to the value of the column to which the predicate is : /// to be applied. : /// @return Raw pointer to an IN Bloom filter predicate. The caller owns the : /// predicate until it is passed into KuduScanner::AddConjunctPredicate(). : /// In the case of > Could you investigate it further as a follow-on item of work? We have an op Ack http://gerrit.cloudera.org:8080/#/c/15122/3/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/15122/3/src/kudu/client/client.cc@982 PS3, Line 982: // Empty vector of bloom filters will select all rows. Hence disallowed. > But that's only if lower_bound_inclusive/upper_bound_exclusive are null, ri Good catch. Fixed. http://gerrit.cloudera.org:8080/#/c/15122/3/src/kudu/client/predicate-test.cc File src/kudu/client/predicate-test.cc: http://gerrit.cloudera.org:8080/#/c/15122/3/src/kudu/client/predicate-test.cc@1260 PS3, Line 1260: // Create random unique values. > Some of these functions (or variations of them) seem like they could live i There already exists Reservoir sampling so removing SelectRandomValues() and combing the remaining two CreateRandomValues() and CreateRandomExclusive() functions in a common function in random_util.h as a separate change. http://gerrit.cloudera.org:8080/#/c/15122/3/src/kudu/client/predicate-test.cc@1273 PS3, Line 1273: std::vector > Using for this, and for unordered_set. Done http://gerrit.cloudera.org:8080/#/c/15122/3/src/kudu/client/scan_predicate.h File src/kudu/client/scan_predicate.h: http://gerrit.cloudera.org:8080/#/c/15122/3/src/kudu/client/scan_predicate.h@111 PS3, Line 111: static Status NewBloomFilter(int num_keys, > This might be difficult to extend/change in the future. What do you think o Good point. Done. http://gerrit.cloudera.org:8080/#/c/15122/3/src/kudu/client/scan_predicate.h@125 PS3, Line 125: /// Clone the Bloom filter. : /// : /// @return Raw pointer to the cloned Bloom filter. Caller owns the Bloom filter till : /// it's passed to @c KuduTable::NewInBloomFilterPredicate() : KuduBloomFilter* Clone() const; > Why should this be part of the public API? When would a third party applica Good catch. InBloomFilterPredicateData as friend class this need not be public. -- To view, visit http://gerrit.cloudera.org:8080/15122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4aa235a4c933ebce0bf3ec7fcb135098eccc4ea4 Gerrit-Change-Number: 15122 Gerrit-PatchSet: 3 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 05 Feb 2020 22:36:14 +0000 Gerrit-HasComments: Yes
