Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10983 )
Change subject: Kudu-1291 Efficiently support predicates on non-prefix key components ...................................................................... Patch Set 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc File src/kudu/tablet/index_skipscan-test.cc: http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@56 PS4, Line 56: kRandom the random tests here are not very random. Can you add an actually-random test case that generates random values? You could do something like this pseudocode: Random random(SeedRandom()); int predicate_val = random.NextInt(); num_matching = GenerateData(random, num_rows, predicate_col_id, predicate_val); // Generates and inserts given number of rows using the given PRNG, returns # rows generated that match the predicate_val on predicate_col. Spec spec = GeneratePredicate(predicate_col_id, predicate_val); ASSERT_EQ(num_matching, ScanWithSpec(spec)); // Scan with given spec and return the number of matching results. The cool thing about SeedRandom() is that if you see a failure you can reproduce the same pseudorandom sequence by passing --test_random_seed to your test when running it (it logs the random seed generated each time it's called) http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@145 PS4, Line 145: int32_t kNumDistinctP1 = 2; : int16_t kNumDistinctP2 = 10; : int kNumDistinctP3 = 5; : int8_t kNumDistinctP4 = 1; : int8_t kNumDistinctP5 = 20; declare all of these variables const http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@163 PS4, Line 163: int32_t int16_t for p2 here and in the below loops http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@226 PS4, Line 226: //Only 1 row inserted nit: formatting, should be: // Only 1 row inserted. http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@253 PS4, Line 253: for (int i = 1; i <= 1; i++) { remove this http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@255 PS4, Line 255: i+1 style nit: please put spaces around infix operators http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@275 PS4, Line 275: p1 ++ style nit: please no spaces before postfix operators http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@297 PS4, Line 297: style nit: collapse >1 consecutive empty lines into a single empty line so more code fits on the screen, here and elsewhere http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@306 PS4, Line 306: void ScanTablet(ScanSpec *spec, vector<string> *results, const char *descr) { > warning: parameter 'descr' is unused [misc-unused-parameters] see tidy bot's suggestion, or remove descr http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@312 PS4, Line 312: /*for (const string &str : *results) { : LOG(INFO) << str; : }*/ remove this http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/index_skipscan-test.cc@551 PS4, Line 551: default: { why do we have a default case? let's just also give this one a name -- To view, visit http://gerrit.cloudera.org:8080/10983 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I230cd5a288e28ace796b352a603e0d1bcc1e4e0f Gerrit-Change-Number: 10983 Gerrit-PatchSet: 4 Gerrit-Owner: Anupama Gupta <anupama.gu...@cloudera.com> Gerrit-Reviewer: Anupama Gupta <anupama.gu...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 01 Aug 2018 00:49:13 +0000 Gerrit-HasComments: Yes