Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16036 )
Change subject: WIP [perf] KUDU-3140 Heuristics to disable predicate evaluation for Bloom filter ...................................................................... Patch Set 3: (12 comments) http://gerrit.cloudera.org:8080/#/c/16036/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16036/3//COMMIT_MSG@14 PS3, Line 14: for every 16 blocks Do you know how large these blocks are by default? Just want to make sure we are comparing apples to apples here. http://gerrit.cloudera.org:8080/#/c/16036/3//COMMIT_MSG@26 PS3, Line 26: TODO: Would also be nice to write a test that leverages a tablet and some string data. 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@684 PS3, Line 684: public: nit: I don't really see why these all need to be a part of the same test class. Maybe consider inlining the tests, and sticking the helper methods in an anonymous namespace or leaving them as static (non-class) methods. http://gerrit.cloudera.org:8080/#/c/16036/3/src/kudu/common/generic_iterators-test.cc@701 PS3, Line 701: google::FlagSaver saver; Tests have a member flagsaver already, so we don't need this. See test_util.h http://gerrit.cloudera.org:8080/#/c/16036/3/src/kudu/common/generic_iterators-test.cc@739 PS3, Line 739: << "I nit: spacing, here and elsewhere http://gerrit.cloudera.org:8080/#/c/16036/3/src/kudu/common/generic_iterators-test.cc@807 PS3, Line 807: // Despite being a static constexpr integer, this static variable needs explicit declaration : // to avoid linker error. Not sure why. Should probably either figure out why, or make it static const or somesuch before merging. http://gerrit.cloudera.org:8080/#/c/16036/3/src/kudu/common/generic_iterators.cc File src/kudu/common/generic_iterators.cc: http://gerrit.cloudera.org:8080/#/c/16036/3/src/kudu/common/generic_iterators.cc@1282 PS3, Line 1282: bool disableable_ineffective_predicate = : effectiveness_ctx != nullptr && !effectiveness_ctx->IsPredicateEnabled(); : // True: a disableable predicate that is determined to be effective. : // False: a non-disableable predicate or a disableable predicate that is determined to be : // ineffective. : bool disableable_effective_predicate = nit: I think the names would be a bit easier to follow as "disableable_predicate_disabled" and "disableable_predicate_enabled" respectively. It seems easy enough to grasp that we'll disable ineffective predicates, so let's focus on the "disable" part. I also think we can do without the explicit True/False comments too in that case, and just mention that we might disable ineffective predicates for the entirety of the iteration. http://gerrit.cloudera.org:8080/#/c/16036/3/src/kudu/common/generic_iterators.cc@1288 PS3, Line 1288: effectiveness_ctx != nullptr nit: can drop the != nullptr, same elsewhere http://gerrit.cloudera.org:8080/#/c/16036/3/src/kudu/common/predicate_effectiveness.h File src/kudu/common/predicate_effectiveness.h: http://gerrit.cloudera.org:8080/#/c/16036/3/src/kudu/common/predicate_effectiveness.h@37 PS3, Line 37: ColumnPredicate* pred; nit: could be const http://gerrit.cloudera.org:8080/#/c/16036/3/src/kudu/common/predicate_effectiveness.h@58 PS3, Line 58: track where nit: add a comma between these, otherwise it's easy to misread this http://gerrit.cloudera.org:8080/#/c/16036/3/src/kudu/common/predicate_effectiveness.h@87 PS3, Line 87: 16 (default) blocks, ratio of rows rejected by a predicate. : // If the rejection ratio is less than 10% nit: these values might change, so how about referring to these values by their flagnames? http://gerrit.cloudera.org:8080/#/c/16036/3/src/kudu/common/predicate_effectiveness.cc File src/kudu/common/predicate_effectiveness.cc: http://gerrit.cloudera.org:8080/#/c/16036/3/src/kudu/common/predicate_effectiveness.cc@59 PS3, Line 59: id nit: idx Also consider `idx_and_pred_ctx` or somesuch (I find it convenient enough to use it in most places) -- 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: 3 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: Fri, 12 Jun 2020 05:22:43 +0000 Gerrit-HasComments: Yes
