Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15424 )
Change subject: [client] Add C++ API to accept BlockBloomFilter directly ...................................................................... Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/15424/1/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/15424/1/src/kudu/client/client.cc@1025 PS1, Line 1025: reinterpret_cast<BlockBloomFilterBufferAllocatorIf*>(const_cast<uint8_t*>(allocator.data())); > You can call mutable_data() instead of data(); it does the const_cast for y Done. Though one downside is changing types of allocator and bloom_filters to pass-by-value. http://gerrit.cloudera.org:8080/#/c/15424/1/src/kudu/client/scan_predicate-internal.h File src/kudu/client/scan_predicate-internal.h: http://gerrit.cloudera.org:8080/#/c/15424/1/src/kudu/client/scan_predicate-internal.h@147 PS1, Line 147: // This class is designed to accept BlockBloomFilter directly as raw pointer : // from consumers like Impala. In such a case, the data is owned by the caller and not : // by the instance of this class. So storing raw pointers and not destructing the pointers would : // have worked fine. However for the case when predicate data is Clone()'d the internal data : // is owned by the instance of this class. Hence using smart pointers with custom deleter : // to keep track of ownership. > This is a useful comment, but I think it'd be better placed on DirectBloomF Ideally this class shouldn't take ownership. This comment explains why Clone()'ing flips the ownership from the caller to this class. So it's best kept close to the internal data structures in this class. So instead of moving this comment, I added a line for the deleter above and linked the two. Other option I explored is moving the deleter inside and making it a nested struct. Despite shortening the name of the deleter, from the caller perspective in client.cc the name with scope resolution appears way too long. Since this is internal header and not exposed I don't see a strong need to make the deleter nested struct. -- To view, visit http://gerrit.cloudera.org:8080/15424 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea85e3da8fe55f78e49a80439e6af723aa3d970b Gerrit-Change-Number: 15424 Gerrit-PatchSet: 1 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Fri, 13 Mar 2020 20:17:40 +0000 Gerrit-HasComments: Yes
