Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16036 )
Change subject: [perf] KUDU-3140 Heuristics to disable predicate evaluation for Bloom filter ...................................................................... Patch Set 4: (3 comments) Looking good! One more test suggestion, otherwise basically looks good to go. http://gerrit.cloudera.org:8080/#/c/16036/4/src/kudu/client/predicate-test.cc File src/kudu/client/predicate-test.cc: http://gerrit.cloudera.org:8080/#/c/16036/4/src/kudu/client/predicate-test.cc@1513 PS4, Line 1513: TEST_F(PredicateTest, TestDisabledBloomFilterWithRepeatedStrings) { Just to make sure we have all our iterators covered, could also add a case where the MRSs gets flushed, and when all the values are updated (i.e. in DMS or deltafiles)? IIRC those cases also exercise top-level iterators. http://gerrit.cloudera.org:8080/#/c/16036/3/src/kudu/common/generic_iterators-test.cc File src/kudu/common/generic_iterators-test.cc: http://gerrit.cloudera.org:8080/#/c/16036/3/src/kudu/common/generic_iterators-test.cc@807 PS3, Line 807: vector<int64_t> ints; : auto bf_pred = CreateBloomFilterPredi > Done Thanks for the explanation! Very odd.. here's a relevant stack overflow post: https://stackoverflow.com/questions/8016780/undefined-reference-to-static-constexpr-char http://gerrit.cloudera.org:8080/#/c/16036/4/src/kudu/common/generic_iterators-test.cc File src/kudu/common/generic_iterators-test.cc: http://gerrit.cloudera.org:8080/#/c/16036/4/src/kudu/common/generic_iterators-test.cc@819 PS4, Line 819: ASSERT_EQ(0, spec.predicates().size()) : < nit: hmm typically we treat ASSERTs as function calls w.r.t spacing, which the GSG mentions should be either aligned with the above line, or at 4 spaces https://google.github.io/styleguide/cppguide.html#Function_Calls -- To view, visit http://gerrit.cloudera.org:8080/16036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10197800a01a1b34c7821ac879caf8d272cab8dd Gerrit-Change-Number: 16036 Gerrit-PatchSet: 4 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 20 Jun 2020 03:29:04 +0000 Gerrit-HasComments: Yes
