Anupama Gupta 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: (22 comments) Please review the changes. http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/cfile/cfile_reader.cc@785 PS4, Line 785: Slice ret; > add: DCHECK_EQ(nullptr, validx_iter_); Added a check for this. Actually no, I think there is no guarantee that value index will always be string. I have added a flag to invoke this function (set_current_value) now . I wonder if there is an alternative to deal with this. http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.h File src/kudu/tablet/cfile_set.h: http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.h@204 PS4, Line 204: // This function is used to place the validx_iter_ at the next greater "prefix_key". > nit: you should try to maintain the same order in the header file as the .c Done http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.h@283 PS4, Line 283: skip_scan_enabled_ > rename to use_skip_scan_ because this is confusingly similar to the gflag n Done http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.h@286 PS4, Line 286: suffix_key_pred_value_ > rename to skip_scan_predicate_value_ Done http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.h@289 PS4, Line 289: suffix_key_column_id_ > rename to skip_scan_predicate_column_id_ Done http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.h@291 PS4, Line 291: // Row id of the last entry of "suffix_key" predicate value wrt to the : // "prefix_key" currently pointed to by validx_iter_ > use something like this as the description: Done http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.h@293 PS4, Line 293: int > make this an int64_t Done http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc@538 PS4, Line 538: if (skip_scan_enabled_ && (static_cast<int>(cur_idx_) > suffix_key_upper_bound_idx_)) { > Can you break this out into its own function? Done http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc@540 PS4, Line 540: bool suffix_key_match, continue_seeking_next_prefix = false; > nit: declare these variables separately Done http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc@542 PS4, Line 542: Status next_entry_match, suffix_key_upper_bound_match; > Can we avoid maintaining so much state outside the loop? Done http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc@602 PS4, Line 602: continue_seeking_next_prefix = true; > not indented Done 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 t Thanks for this suggestion. I have added the random tests now. 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 Done 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 Done 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: Done. 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 Done 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 Done 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 Done 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 Done 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] Done 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 Done 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 Done. Removed this test. -- 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 <[email protected]> Gerrit-Reviewer: Anupama Gupta <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 02 Aug 2018 00:11:51 +0000 Gerrit-HasComments: Yes
