ZhangYao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11100 )
Change subject: Implement BloomFilter Predicate in server side. ...................................................................... Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/11100/2/src/kudu/common/column_predicate.h File src/kudu/common/column_predicate.h: http://gerrit.cloudera.org:8080/#/c/11100/2/src/kudu/common/column_predicate.h@97 PS2, Line 97: // Either (but not both) of the bounds may be a nullptr to indicate an : // unbounded range on that end. > I'm not sure range queries should be able to support BloomFilters. Is InBlo Actually users are not suppose to use both range and bloomfilter for the same column , but this is possible to happen so I should handle this case and merge all the predicates for the same column. http://gerrit.cloudera.org:8080/#/c/11100/2/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/11100/2/src/kudu/common/common.proto@351 PS2, Line 351: // The bloomfilter fo > One of the things that I think might be confusing in the future is there is I name it BloomFilter to be consistent with other predicates message such as Range. http://gerrit.cloudera.org:8080/#/c/11100/2/src/kudu/common/common.proto@352 PS2, Line 352: ssage BloomFilter { : optional int32 nhash = 1; : // We currently use CityHash backend > It's a bit unclear what these fields are. Mind adding comments explaining? Done http://gerrit.cloudera.org:8080/#/c/11100/2/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: http://gerrit.cloudera.org:8080/#/c/11100/2/src/kudu/common/wire_protocol.cc@412 PS2, Line 412: // Copies a predicate bloom filter data from 'bf_src' into 'bf_dst'. : void CopyPredicateBloomFilterToPB(const ColumnPredicate::BloomFilterInner& bf_src, : ColumnPredicatePB::BloomFilter* bf_dst) { : bf_dst->set_nhash(bf_src.nhash()); : const void* src; : size_t size; : src = bf_src.bloom_data().data(); : > This would be clearer if it took a BloomFilterInner instead of a Slice. Sam Done http://gerrit.cloudera.org:8080/#/c/11100/2/src/kudu/tablet/cfile_set-test.cc File src/kudu/tablet/cfile_set-test.cc: http://gerrit.cloudera.org:8080/#/c/11100/2/src/kudu/tablet/cfile_set-test.cc@104 PS2, Line 104: : ASSERT_OK(rsw.Finish()); : } : > Can you comment what these variables do and how they should be used? Same w Done http://gerrit.cloudera.org:8080/#/c/11100/2/src/kudu/tablet/cfile_set-test.cc@451 PS2, Line 451: DoTestRangeScan(fileset, 2001, 2009); > Would you mind adding a high-level summary of what this test is testing and Done -- 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: 4 Gerrit-Owner: ZhangYao <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: ZhangYao <[email protected]> Gerrit-Comment-Date: Tue, 14 Aug 2018 10:45:31 +0000 Gerrit-HasComments: Yes
