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 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/15122/4/src/kudu/client/scan_predicate.h File src/kudu/client/scan_predicate.h: http://gerrit.cloudera.org:8080/#/c/15122/4/src/kudu/client/scan_predicate.h@127 PS4, Line 127: /// @param [in] num_keys : /// Expected number of elements to be inserted in the Bloom filter. : /// @return Reference to the updated object. : KuduBloomFilterBuilder& num_keys(int num_keys); > There's no point in allowing num_keys to be configured via either construct Done http://gerrit.cloudera.org:8080/#/c/15122/4/src/kudu/client/scan_predicate.h@155 PS4, Line 155: till > Nit: until Done http://gerrit.cloudera.org:8080/#/c/15122/4/src/kudu/client/scan_predicate.h@167 PS4, Line 167: // Default constructor deleted. : KuduBloomFilterBuilder(); > I don't think the compiler will generate one if you omit this, since you pr Done http://gerrit.cloudera.org:8080/#/c/15122/4/src/kudu/client/scan_predicate.cc File src/kudu/client/scan_predicate.cc: http://gerrit.cloudera.org:8080/#/c/15122/4/src/kudu/client/scan_predicate.cc@181 PS4, Line 181: vector<BlockBloomFilter*> block_bloom_filters; > Somewhat unrelated to the current change, but why isn't this a vector<uniqu Done. With original implementation that accepted BlockBloomFilter directly multiple conversions were required when accepting the raw BlockBloomFilter and then while supplying the BlockBloomFilter to the server if stored as unique_ptr. With PIMPL pattern, the conversion are needed, so better to store as unique_ptrs. Also saves some code with not needing explicit destructor. http://gerrit.cloudera.org:8080/#/c/15122/4/src/kudu/client/scan_predicate.cc@220 PS4, Line 220: : KuduBloomFilter::KuduBloomFilter(Data* other_data) : : data_(CHECK_NOTNULL(other_data)) { : } > This is frustrating, but ultimately necessary since we can't put a move con Done. Added comment in the header file. -- 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: 4 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: Sat, 08 Feb 2020 01:00:54 +0000 Gerrit-HasComments: Yes
