Alexey Serbin 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) few nits 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? 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() ? 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? 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: IsColumnPredicateAdaptive() ? 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 explicit PredicateEffectivenessContext(const ColumnPredicate* pred) : pred(pred), rows_read(0), rows_rejected(0), enabled(true) { } 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? http://gerrit.cloudera.org:8080/#/c/16036/6/src/kudu/common/predicate_effectiveness.h@81 PS6, Line 81: return it->second; ditto 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 as expected? Isn't it possible to recover from an error in such case? 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 ? -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 24 Jun 2020 19:11:56 +0000 Gerrit-HasComments: Yes
