Alexey Serbin 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 14: (6 comments) http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc@462 PS12, Line 462: : // Store the predicate column id. : skip_scan_predicate_column_id_ = min_col_id; : : // Store the predicate value. : skip_scan_predicate_value_ = pred_value; : : // Store the cutoff on the number of skip scan seeks. : > Alexey, please let me know if you feel that this change should be reverted. I think it's better to keep it as Andrew requested. After some consideration I realized that's clearer because it's moved out of the cycle at least. http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/tablet/cfile_set.cc@414 PS14, Line 414: int num_key_cols nit: since this is not to change in the code below, add 'const' http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/tablet/cfile_set.cc@576 PS14, Line 576: ( style nit: keep the brace with the name of the method http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/index_skipscan-test.cc File src/kudu/tablet/index_skipscan-test.cc: http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/index_skipscan-test.cc@321 PS12, Line 321: auto pred_p1 = ColumnPredicate::Equality(schema_.column(0), &value_p1); : auto pred_p2 = ColumnPredicate::Equality(schema_.column(1), &value_p2); : spec.AddPredicate(pred_p1); : spec.AddPredicate(pred_p2); : vector<string> results; : NO_FATALS(ScanTablet(&spec, &results, "Exact match on P1 and P2")); : EXPECT_EQ(1, results.size()); : } : break; : : c > Changed to a more uniform layout. Please let me know if this looks good. thanks, lgtm http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/tablet/index_skipscan-test.cc File src/kudu/tablet/index_skipscan-test.cc: PS14: This seems to be a good set of unit tests regarding testing scan using different combinations primary key columns. How about adding a simple unit test to verify that skip scan is enabled/disabled in cfileset iterator? I.e., I was thinking to verify functionality of CFileSet::Iterator::TryEnableSkipScan(). What do you think? http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/tablet/index_skipscan-test.cc@189 PS14, Line 189: const int32_t kNumDistinctP1 = 2; : const int16_t kNumDistinctP2 = 10; : const int kNumDistinctP3 = 5; : const int8_t kNumDistinctP4 = 1; : const int8_t kNumDistinctP5 = 20; nit: all there might be constexpr: constexpr int32_t kNumDistinctP1 = 2; ... -- 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: 14 Gerrit-Owner: Anupama Gupta <anupama.gu...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@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: Tue, 14 Aug 2018 05:47:54 +0000 Gerrit-HasComments: Yes
