Bankim Bhavsar 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 w Thanks for pointing that out. I checked kudu by default uses block of 128 rows. >From Wenzhe, HDFS scanner in Impala uses default block of 1024 rows. To keep the heuristic same as HDFS, the skip block count will need to be 128. (Haven't made the change) 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 Trying to understand the reason before I make the change. This'll allow testing with dict encoded repeated strings and hence whether predicate evaluation is disabled at decoder level? 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 cl Moved the 2 public methods directly as part of the Test outside the class. Using a class helps - Use the parameterized test - Keep the common static const variables confined to the relevant tests instead of making them available for entire file 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 Done 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 Done 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 Done 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_pred Changed the names. Re: True/False At first, it's tempting to simply use negation of first variable for effective case but that would yield unexpected result. So the comment helps for understanding the use. Unless there is strong opposition I'd rather have that comment. 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 Done 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 Done 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 Done 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 t Done 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 Done -- 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 22:43:25 +0000 Gerrit-HasComments: Yes
