[kudu-CR] Kudu-1291 Efficiently support predicates on non-prefix key components

["Mike Percy (Code Review)" <ge...@cloudera.org>]

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 <an...@cloudera.com>
Gerrit-Reviewer: Anupama Gupta <an...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 01 Aug 2018 00:49:13 +0000
Gerrit-HasComments: Yes