Adar Dembo 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 this table." 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 test set membership with a possibility of false positive matches." http://gerrit.cloudera.org:8080/#/c/15122/1/src/kudu/client/client.h@1093 PS1, Line 1093: BoomFilter BloomFilter http://gerrit.cloudera.org:8080/#/c/15122/1/src/kudu/client/client.h@1093 PS1, Line 1093: In IN http://gerrit.cloudera.org:8080/#/c/15122/1/src/kudu/client/client.h@1094 PS1, Line 1094: space efficient space-efficient 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 bloom filter. Could the same thing be achieved via separate comparison predicates? 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 BlockBloomFilter directly to clients? This is problematic for several reasons: 1. Our hands are tied much more tightly w.r.t. making changes to those classes. See https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C++ for details. 2. The header files that define those classes must be compatible with pre-C++11 compilers, which means they can't use unique_ptr, map, and other C++11 stuff. They also have to be shipped with the SDK, and the associated symbols need to be tagged with KUDU_EXPORT as needed. Across much of the C++ API we've avoided these pitfalls by PIMPL'ing (Pointer to IMPLementation) the underlying classes with client-only shims. In a few cases (like KuduPartialRow and KuduWriteOperation) we haven't, ostensibly for performance, though it's resulted in some really frustrating issues (see https://issues.apache.org/jira/browse/KUDU-1563?focusedCommentId=16713156&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16713156 for an example). 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 need to use NULL here instead of nullptr. 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. 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) before; do we use it elsewhere? If not, could you forgo 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? 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. -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 29 Jan 2020 00:42:04 +0000 Gerrit-HasComments: Yes
