Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11100 )
Change subject: Implement BloomFilter Predicate in server side. ...................................................................... Patch Set 8: (7 comments) http://gerrit.cloudera.org:8080/#/c/11100/8/src/kudu/common/column_predicate-test.cc File src/kudu/common/column_predicate-test.cc: http://gerrit.cloudera.org:8080/#/c/11100/8/src/kudu/common/column_predicate-test.cc@77 PS8, Line 77: Random rand(SeedRandom()); You shouldn't call this more than once per test. So maybe add it to the fixture? http://gerrit.cloudera.org:8080/#/c/11100/8/src/kudu/common/column_predicate-test.cc@1399 PS8, Line 1399: bfs.push_back(bf1); Still got some usage of push_back in this test instead of emplace_back. http://gerrit.cloudera.org:8080/#/c/11100/6/src/kudu/common/column_predicate.h File src/kudu/common/column_predicate.h: http://gerrit.cloudera.org:8080/#/c/11100/6/src/kudu/common/column_predicate.h@275 PS6, Line 275: : BloomFilterInner() : nhash_(0), hash_algorithm_(CITY_HASH) {} : : const Slice& bloom_data() const { : return bloom_data_; : } : : size_t nhash() const { : return nhash_; : } : : HashAlgorithmInBloomFilter hash_algorithm() const { : return hash_algorithm_; : } : : void set_nhash(size_t nhash) { : nhash_ = nhash; : } : : void set_bloom_data(Slice bloom_data) { : bloom_data_ = bloom_data; : } : : v > Emmm I simply try to use setters and getters to let the calls look uniform. Gotcha. Yeah, I agree, if you want to keep the setters/getters as an abstraction, switch to a class so that default visibility is private. Or just drop the setters/getters. Whatever you prefer. http://gerrit.cloudera.org:8080/#/c/11100/6/src/kudu/common/column_predicate.cc File src/kudu/common/column_predicate.cc: http://gerrit.cloudera.org:8080/#/c/11100/6/src/kudu/common/column_predicate.cc@275 PS6, Line 275: predicate_type_ = PredicateType::Equality; > Yes, I just simply copy the Range simplify part. Should I modify the Range Yes, please do. http://gerrit.cloudera.org:8080/#/c/11100/6/src/kudu/common/wire_protocol-test.cc File src/kudu/common/wire_protocol-test.cc: http://gerrit.cloudera.org:8080/#/c/11100/6/src/kudu/common/wire_protocol-test.cc@555 PS6, Line 555: TEST_F(WireProtocolTest, TestColumnPredicateBloomFilterWithBound) { > The BF test is juxtaposed with InList test above, It is necessary to do thi Yeah, decomposing the one for InList would be great too. Test code tends to be verbose and it's easy for our eyes to glaze over when reviewing it, so anything you can do to make it shorter means we're more likely to give it the same detail in code review as we would non-test code. http://gerrit.cloudera.org:8080/#/c/11100/8/src/kudu/util/bloom_filter.h File src/kudu/util/bloom_filter.h: http://gerrit.cloudera.org:8080/#/c/11100/8/src/kudu/util/bloom_filter.h@67 PS8, Line 67: case CITY_HASH: // Default use city hash. Nit: don't need the comment here; it's obvious from the default value. http://gerrit.cloudera.org:8080/#/c/11100/6/src/kudu/util/bloom_filter.h File src/kudu/util/bloom_filter.h: http://gerrit.cloudera.org:8080/#/c/11100/6/src/kudu/util/bloom_filter.h@57 PS6, Line 57: explicit BloomKeyProbe(const Slice &key, HashAlgorithmInBloomFilter hash_algorithm = CITY_HASH) > I chose to use constructor with default value. I can also simply delete the Default values are fine too; I just wanted to make sure the repeated code is consolidated. -- To view, visit http://gerrit.cloudera.org:8080/11100 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I62c2de42667d0255d94e19db773240f7f9ee636c Gerrit-Change-Number: 11100 Gerrit-PatchSet: 8 Gerrit-Owner: ZhangYao <triplesheep0...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: ZhangYao <triplesheep0...@gmail.com> Gerrit-Comment-Date: Mon, 24 Sep 2018 18:52:19 +0000 Gerrit-HasComments: Yes
