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 4: (3 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, Here and below you don't need the rvalue reference; could just pass std::vector<...> as-is and still move it. http://gerrit.cloudera.org:8080/#/c/15034/3/src/kudu/util/block_bloom_filter.h File src/kudu/util/block_bloom_filter.h: http://gerrit.cloudera.org:8080/#/c/15034/3/src/kudu/util/block_bloom_filter.h@78 PS3, Line 78: // Initialize the internal data structures using the supplied arguments. : // Useful for de-serializing the BlockBloomFilter. : Status Init(int log_space_bytes, const void* src_data, size_t src_len, bool always_false); > Passing the entire kudu::ColumnPredicatePB_BlockBloomFilter would require i Good point. What do you think about moving the PB representation of BlockBloomFilter into kudu_util? There's some precedence for that (see hash.proto). http://gerrit.cloudera.org:8080/#/c/15034/3/src/kudu/util/block_bloom_filter.cc File src/kudu/util/block_bloom_filter.cc: http://gerrit.cloudera.org:8080/#/c/15034/3/src/kudu/util/block_bloom_filter.cc@108 PS3, Line 108: > Done Ths Init variant calls InitInternal (memset on L91) and then calls memcpy (L115). Likewise, the other Init variant calls InitInternal (memset) and does another memset on L98. So it doesn't seem like we've gained here; if anything, we're duplicating work more now. Am I missing something? -- 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: 4 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: Thu, 16 Jan 2020 04:35:30 +0000 Gerrit-HasComments: Yes
