Adar Dembo 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 understand but using rvalue reference as type for bfs forces caller to pa I see. That runs afoul of the Google Style Guide's rules on rvalue references though: https://google.github.io/styleguide/cppguide.html#Rvalue_references If you grep across the Kudu codebase, all of the rvalue reference argument types are either for move constructors, move assignment operators, or perfect forwarding. 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? 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 other PROTOBUF_GENERATE_CPP do it in this file. 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, regardless of the value of always_false? -- 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 00:31:31 +0000 Gerrit-HasComments: Yes
