Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15373 )
Change subject: [util] Import "Or" function to BlockBloomFilter from Impala ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/15373/1/src/kudu/util/block_bloom_filter-test.cc File src/kudu/util/block_bloom_filter-test.cc: http://gerrit.cloudera.org:8080/#/c/15373/1/src/kudu/util/block_bloom_filter-test.cc@304 PS1, Line 304: for (int i = 11; i < 50; ++i) ASSERT_TRUE(bf1->Find(i)); if this fails, how to know what 'i' it was? http://gerrit.cloudera.org:8080/#/c/15373/1/src/kudu/util/block_bloom_filter.h File src/kudu/util/block_bloom_filter.h: http://gerrit.cloudera.org:8080/#/c/15373/1/src/kudu/util/block_bloom_filter.h@153 PS1, Line 153: Or For more elegant syntax alternative, maybe introduce BlockBloomFilter& operator |=(const BlockBloomFilter& other) as well? http://gerrit.cloudera.org:8080/#/c/15373/1/src/kudu/util/block_bloom_filter.h@229 PS1, Line 229: AVX AVX2 ? http://gerrit.cloudera.org:8080/#/c/15373/1/src/kudu/util/block_bloom_filter.cc File src/kudu/util/block_bloom_filter.cc: http://gerrit.cloudera.org:8080/#/c/15373/1/src/kudu/util/block_bloom_filter.cc@287 PS1, Line 287: DCHECK_NE(&other, kAlwaysTrueFilter); Why is this disallowed? Could you add a short explanation? http://gerrit.cloudera.org:8080/#/c/15373/1/src/kudu/util/block_bloom_filter.cc@291 PS1, Line 291: DCHECK_EQ BTW, is this Or() method protected from inputs from the wild that might not come under radar in DEBUG case? In other words, why DCHECK_EQ() is enough and CHECK_EQ() is not needed here? http://gerrit.cloudera.org:8080/#/c/15373/1/src/kudu/util/block_bloom_filter_avx2.cc File src/kudu/util/block_bloom_filter_avx2.cc: http://gerrit.cloudera.org:8080/#/c/15373/1/src/kudu/util/block_bloom_filter_avx2.cc@86 PS1, Line 86: DCHECK_EQ Is it guaranteed that debug builds provide enough coverage to make sure n % kAVXRegisterBytes == 0 in release builds? -- To view, visit http://gerrit.cloudera.org:8080/15373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibe5f9311f73dcff883dd2cce18fd558e7d57d14f Gerrit-Change-Number: 15373 Gerrit-PatchSet: 1 Gerrit-Owner: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Fri, 06 Mar 2020 07:28:08 +0000 Gerrit-HasComments: Yes
