Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15122 )
Change subject: [client] KUDU-2483 Add InBloomFilter predicate to C++ client ...................................................................... Patch Set 2: (12 comments) 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@1084 PS1, Line 1084: /// Create a new IN BloomFilter predicate which can be used for scanners on : /// this table. > "Create a new IN bloom filter predicate which can be used for scanners on t Changed to "IN Bloom filter" here and other places. http://gerrit.cloudera.org:8080/#/c/15122/1/src/kudu/client/client.h@1087 PS1, Line 1087: /// BloomFilter is a space-efficient probabilistic data-structure used to : /// test set membership with a possibility of false positive matches. > "A bloom filter is a space-efficient probabilistic data structure used to t Done http://gerrit.cloudera.org:8080/#/c/15122/1/src/kudu/client/client.h@1093 PS1, Line 1093: BoomFilter > BloomFilter Changed to "IN Bloom filter" here and other places. http://gerrit.cloudera.org:8080/#/c/15122/1/src/kudu/client/client.h@1093 PS1, Line 1093: In > IN Done http://gerrit.cloudera.org:8080/#/c/15122/1/src/kudu/client/client.h@1094 PS1, Line 1094: space efficient > space-efficient Done http://gerrit.cloudera.org:8080/#/c/15122/1/src/kudu/client/client.h@1104 PS1, Line 1104: /// @param [in] lower_bound_inclusive : /// @param [in] upper_bound_exclusive : /// Optional [lower_bound, upper_bound) filters where type of the bound : /// must correspond to the value of the column to which the predicate is : /// to be applied. > Not clear how these relate to the concept of testing set membership in a bl This is from the original implementation of the wire protocol for bloom filter predicate. Looks like same thing could be achieved using separate comparison predicate. But then there is existing merging logic as well. Keeping this item open for now. http://gerrit.cloudera.org:8080/#/c/15122/1/src/kudu/client/client.h@1117 PS1, Line 1117: BlockBloomFilterBufferAllocatorIf* allocator, : std::vector<BlockBloomFilter*> bloom_filters, > Are you sure we want to expose BlockBloomFilterBufferAllocatorIf and BlockB Thanks for those pointers, Adar. Added a new exportable KuduBloomFilter class in scan_predicate.h public header file that contains pointers to the allocator and actual BlockBloomFilter using the PIMPL design pattern. http://gerrit.cloudera.org:8080/#/c/15122/1/src/kudu/client/client.h@1119 PS1, Line 1119: KuduValue* lower_bound_inclusive = nullptr, : KuduValue* upper_bound_exclusive = nullptr); > The public C++ API has to be usable with pre-C++11 compilers, so you'll nee Done http://gerrit.cloudera.org:8080/#/c/15122/1/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/15122/1/src/kudu/client/client.cc@973 PS1, Line 973: std:: > Nit: don't need. Done http://gerrit.cloudera.org:8080/#/c/15122/1/src/kudu/client/client.cc@974 PS1, Line 974: KuduValue* lower_bound_inclusive /* = nullptr */, : KuduValue* upper_bound_exclusive /* = nullptr */) > I haven't seen this style (inline comments for default argument values) bef I recall this somewhere in Kudu code but can't find it now, so removing it. http://gerrit.cloudera.org:8080/#/c/15122/1/src/kudu/client/scan_predicate.cc File src/kudu/client/scan_predicate.cc: http://gerrit.cloudera.org:8080/#/c/15122/1/src/kudu/client/scan_predicate.cc@168 PS1, Line 168: client:: > Why is this needed? Done http://gerrit.cloudera.org:8080/#/c/15122/1/src/kudu/client/scan_predicate.cc@203 PS1, Line 203: // Interface doesn't allow for returning error, so throwing an exception. : throw std::runtime_error("Error cloning BlockBloomFilter, " + s.ToString()); > Do a CHECK_OK(s) instead of this. Done -- 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: 2 Gerrit-Owner: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 31 Jan 2020 23:06:44 +0000 Gerrit-HasComments: Yes
