Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15450 )
Change subject: [util] Minor changes in BlockBloomFilter requested by Impala ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/15450/2/src/kudu/util/block_bloom_filter.h File src/kudu/util/block_bloom_filter.h: http://gerrit.cloudera.org:8080/#/c/15450/2/src/kudu/util/block_bloom_filter.h@263 PS2, Line 263: // Function pointers initialized just once to avoid run-time cost : // in hot-path of Find, Insert and Or operations. : static decltype(&BlockBloomFilter::BucketInsert) bucket_insert_func_ptr_; : static decltype(&BlockBloomFilter::BucketFind) bucket_find_func_ptr_; : static decltype(&BlockBloomFilter::OrEqualArrayNoAVX2) or_equal_array_func_ptr_; Do these need to be declared in the header? I don't see them used. http://gerrit.cloudera.org:8080/#/c/15450/2/src/kudu/util/block_bloom_filter.cc File src/kudu/util/block_bloom_filter.cc: http://gerrit.cloudera.org:8080/#/c/15450/2/src/kudu/util/block_bloom_filter.cc@48 PS2, Line 48: // Flag used to initialize the static function pointers for the BlockBloomFilter class. : static std::once_flag g_init_func_ptrs_flag; Move it into the anonymous namespace and you won't need to declare it static. http://gerrit.cloudera.org:8080/#/c/15450/2/src/kudu/util/hash_util.h File src/kudu/util/hash_util.h: http://gerrit.cloudera.org:8080/#/c/15450/2/src/kudu/util/hash_util.h@139 PS2, Line 139: // Can't use LOG(FATAL)/CHECK() since including glog/logging.h causes problems : // with code-gen in Impala. Depending on whether this is on a hot path or not, you could also move it to a .cc file (and then it's presumably safe to use glog?) -- To view, visit http://gerrit.cloudera.org:8080/15450 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5944f80f4c071ce787eded3f5b41d3bc56560cd0 Gerrit-Change-Number: 15450 Gerrit-PatchSet: 2 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: Mon, 16 Mar 2020 22:10:21 +0000 Gerrit-HasComments: Yes
