Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15034 )
Change subject: KUDU-2483 Integrate BlockBloomFilter with ColumnPredicate on server side ...................................................................... Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/15034/4/src/kudu/common/column_predicate.h File src/kudu/common/column_predicate.h: http://gerrit.cloudera.org:8080/#/c/15034/4/src/kudu/common/column_predicate.h@152 PS4, Line 152: std::vector<BlockBloomFilterInner>&& bfs, > I see. That runs afoul of the Google Style Guide's rules on rvalue referenc Agreeing a bit reluctantly. Though one benefit I see is that it makes writing unit tests easier and avoids creating copies :) http://gerrit.cloudera.org:8080/#/c/15034/5/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: http://gerrit.cloudera.org:8080/#/c/15034/5/src/kudu/common/wire_protocol.cc@633 PS5, Line 633: if (!bf_src.has_log_space_bytes() || !bf_src.has_bloom_data() || : !bf_src.has_hash_algorithm() || bf_src.hash_algorithm() == UNKNOWN_HASH || : !bf_src.has_always_false()) { : return Status::InvalidArgument("Invalid in bloom filter predicate on column: " : "missing bloom filter details", col.name()); : } > Should this be moved into BlockBloomFilter::InitFromPB? Thanks for bringing this up. BlockBloomFilter doesn't contain the hash_algorithm and hash_seed fields and hence these checks were outside in the caller here. I refactored the code to include hash_algorithm and hash_seed inside BlockBloomFilter. This way BlockBloomFilter can be completely initialized from the protobuf message and makes it clean. Also removes the need of BloomFilterInner which looked like a crutch. Now that hash_algorithm and hash_seed are known to BlockBloomFilter, added Insert() and Find() convenience functions in BlockBloomFilter that takes a Slice and hashes prior to insertion/lookup. To keep BlockBloomFilter usable from Impala, direct insertion and lookup using 32-bit hash values is still made available. Also there is a slight performance optimization in case hash of the key is already known. There is possible risk of caller using direct 32-bit hash values but specifies different hash_algorithm to the BlockBloomFilter. To reduce that risk, made the constructor explicitly specify the hash function and seed. http://gerrit.cloudera.org:8080/#/c/15034/5/src/kudu/util/CMakeLists.txt File src/kudu/util/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/15034/5/src/kudu/util/CMakeLists.txt@21 PS5, Line 21: > Can you fix the indentation for the continuation lines? Check out how the o Done http://gerrit.cloudera.org:8080/#/c/15034/5/src/kudu/util/block_bloom_filter.cc File src/kudu/util/block_bloom_filter.cc: http://gerrit.cloudera.org:8080/#/c/15034/5/src/kudu/util/block_bloom_filter.cc@113 PS5, Line 113: memcpy(directory_, bf_src.bloom_data().data(), bf_src.bloom_data().size()); > Shouldn't we just do this copy to faithfully respect the state of bf_src, r Done -- To view, visit http://gerrit.cloudera.org:8080/15034 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ecfd67e9c5fbe459c5b4aed91e0be2a194d433a Gerrit-Change-Number: 15034 Gerrit-PatchSet: 5 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: Yao Xu <[email protected]> Gerrit-Reviewer: ZhangYao <[email protected]> Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Fri, 17 Jan 2020 23:15:14 +0000 Gerrit-HasComments: Yes
