Bankim Bhavsar 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 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/16036/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16036/6//COMMIT_MSG@43 PS6, Line 43: TODO: Run TPCH with latest patchset to ascertain no changes in results : before merging. > I'm curious: has it been done? Done. https://gist.github.com/bbhavsar/0a773359b9225f014d353759a535c5be http://gerrit.cloudera.org:8080/#/c/16036/6/src/kudu/common/generic_iterators-test.cc File src/kudu/common/generic_iterators-test.cc: http://gerrit.cloudera.org:8080/#/c/16036/6/src/kudu/common/generic_iterators-test.cc@671 PS6, Line 671: CheckColumnPredicatesAreEqual(vector<ColumnPredicate>({ c_equality, b_equality, a_range }), : GetIteratorPredicatesForTests(iter)); > Wrap into NO_FATALS() ? Done http://gerrit.cloudera.org:8080/#/c/16036/6/src/kudu/common/generic_iterators-test.cc@714 PS6, Line 714: bool all_values > Does it make sense to document the semantics of this parameter? Done http://gerrit.cloudera.org:8080/#/c/16036/6/src/kudu/common/predicate_effectiveness.h File src/kudu/common/predicate_effectiveness.h: http://gerrit.cloudera.org:8080/#/c/16036/6/src/kudu/common/predicate_effectiveness.h@36 PS6, Line 36: IsColumnPredicateDisableable > Might the following be a better choice for the name of the function: The term "disableable" does sound weird but it make the purpose obvious, i.e. column predicate could be disabled. Adaptive on one hand is a more familiar word but it's not obvious what part of column predicate is adaptive. Adaptive would have been more appropriate if the predicate becomes more restrictive or less restrictive based on some feedback, like a moving gauge. So I'm sticking with disableable. http://gerrit.cloudera.org:8080/#/c/16036/6/src/kudu/common/predicate_effectiveness.h@44 PS6, Line 44: explicit PredicateEffectivenessContext(const ColumnPredicate* pred) : : pred(pred), : rows_read(0), : rows_rejected(0), : enabled(true) { : } > nit: per code style guide it should be formatted a bit different Done http://gerrit.cloudera.org:8080/#/c/16036/6/src/kudu/common/predicate_effectiveness.h@75 PS6, Line 75: return it->second; > What happens if in non-debug build an invalid index is specified? Done http://gerrit.cloudera.org:8080/#/c/16036/6/src/kudu/common/predicate_effectiveness.h@81 PS6, Line 81: return it->second; > ditto Done http://gerrit.cloudera.org:8080/#/c/16036/6/src/kudu/mini-cluster/internal_mini_cluster.cc File src/kudu/mini-cluster/internal_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/16036/6/src/kudu/mini-cluster/internal_mini_cluster.cc@280 PS6, Line 280: void > Why not to return Status instead of calling CHECK() if something goes not a Done http://gerrit.cloudera.org:8080/#/c/16036/6/src/kudu/tablet/tablet_metrics.cc File src/kudu/tablet/tablet_metrics.cc: http://gerrit.cloudera.org:8080/#/c/16036/6/src/kudu/tablet/tablet_metrics.cc@85 PS6, Line 85: disableable > adaptive ? Same reasoning as above about adaptive v/s disableable. -- 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: 6 Gerrit-Owner: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 25 Jun 2020 23:00:36 +0000 Gerrit-HasComments: Yes
