Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15913 )
Change subject: [perf] Check range predicate first while evaluating Bloom filter predicate ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/15913/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15913/1//COMMIT_MSG@18 PS1, Line 18: Tests: > nit: is this a best-case scenario? average scenario? worst case? If we tune Disregarding the data type of the column, I think this is the best case scenario considering range predicate checks prevents any checks to Bloom filter. Hashing for larger string/binary values could give better numbers. http://gerrit.cloudera.org:8080/#/c/15913/1/src/kudu/client/predicate-test.cc File src/kudu/client/predicate-test.cc: http://gerrit.cloudera.org:8080/#/c/15913/1/src/kudu/client/predicate-test.cc@1311 PS1, Line 1311: num_false_positive_values_ = num_all_values * kBloomFilterFalsePositiveProb; > warning: narrowing conversion from 'float' to 'int' [bugprone-narrowing-con Done http://gerrit.cloudera.org:8080/#/c/15913/1/src/kudu/client/predicate-test.cc@1350 PS1, Line 1350: auto * > nit: revert the order? Done http://gerrit.cloudera.org:8080/#/c/15913/1/src/kudu/client/predicate-test.cc@1353 PS1, Line 1353: const auto* test_name = ::testing::UnitTest::GetInstance()->current_test_info()->name(); > warning: Value stored to 'test_name' during its initialization is never rea Using CURRENT_TEST_NAME() macro instead. http://gerrit.cloudera.org:8080/#/c/15913/1/src/kudu/client/predicate-test.cc@1354 PS1, Line 1354: strings:: > nit: add a `using` for this? Done http://gerrit.cloudera.org:8080/#/c/15913/1/src/kudu/common/column_predicate.h File src/kudu/common/column_predicate.h: http://gerrit.cloudera.org:8080/#/c/15913/1/src/kudu/common/column_predicate.h@326 PS1, Line 326: return false; : } : } else if (upper_ != nullptr) { : if (DataTypeTraits<PhysicalType>::Compare(cell, this->upper_) >= 0) { : return false; : } : } else if (lower_ != nullptr) { : if (DataTypeTraits<PhysicalType>::Compare(cell, this->lower_) < 0) { : return false; : > nit: spacing for 'return false' Done http://gerrit.cloudera.org:8080/#/c/15913/1/src/kudu/common/column_predicate.h@322 PS1, Line 322: // Check optional lower and upper bound. : if (lower_ != nullptr && upper_ != nullptr) { : if (DataTypeTraits<PhysicalType>::Compare(cell, this->upper_) >= 0 || : DataTypeTraits<PhysicalType>::Compare(cell, this->lower_) < 0) { : return false; : } : } else if (upper_ != nullptr) { : if (DataTypeTraits<PhysicalType>::Compare(cell, this->upper_) >= 0) { : return false; : } : } else if (lower_ != nullptr) { : if (DataTypeTraits<PhysicalType>::Compare(cell, this->lower_) < 0) { : return false; : } : } > nit: would this work: Indeed! http://gerrit.cloudera.org:8080/#/c/15913/1/src/kudu/common/column_predicate.h@349 PS1, Line 349: return false; > nit: spacing Looks like simply importing Google style didn't update the spacing in IDE on Mac. Fixed now. -- To view, visit http://gerrit.cloudera.org:8080/15913 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8451d6ddfe1fbdf307b3e9f2cc23a8d06e655ba3 Gerrit-Change-Number: 15913 Gerrit-PatchSet: 1 Gerrit-Owner: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 14 May 2020 21:05:49 +0000 Gerrit-HasComments: Yes
