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 12: (13 comments) http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/cfile/cfile_reader.h File src/kudu/cfile/cfile_reader.h: http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/cfile/cfile_reader.h@348 PS12, Line 348: const std::string& GetCurrentValue(); Add 'const' modifier for this method. http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/cfile/cfile_reader.h@472 PS12, Line 472: // Value currently pointed to by validx_iter_. : // Currently, it is assumed that CFileSet::Iterator::SkipToNextScan(size_t *remaining) : // has exclusive access to use this variable. Any chance to make update this to keep better level of encapsulation? If not, maybe add TODO here to be more aware of this caveat. http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/cfile/cfile_reader.cc@792 PS12, Line 792: DCHECK(!prepared_blocks_.empty()); What happens if this does not hold in release build? I'm curious why that would not be an option to return non-OK status in that case in addition to DCHECK() ? 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@19 PS12, Line 19: #include <math.h> nit: please use '#include <cmath>' instead and place that among other C++-style include directives below, keeping the list sorted alphabetically. http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc@76 PS12, Line 76: true Per our discussion with Mike today, I thought the idea was to have this disabled in first commit before more-like-real-world testing is done. Am I missing something? http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc@435 PS12, Line 435: std::unordered_map<std::string, ColumnPredicate> nit: you could use 'auto' here. http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc@444 PS12, Line 444: col_id = Could find_column() return -1 here? http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc@462 PS12, Line 462: if (nonfirst_key_pred_exists) { : use_skip_scan_ = true; : : // Store the predicate column id. : skip_scan_predicate_column_id_ = min_col_id; : : // Store the predicate value. : skip_scan_predicate_value_ = pred_value; : } nit: why not to move this under the same scope where the nonfirst_key_pred_exists set to 'true' but after the 'if (col_id < min_col_id)' closure? Doing so, you could get rid of the nonfirst_key_pred_exists, but don't forget to add a comment about the semantics of those updates. http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc@542 PS12, Line 542: * nit: stick the asterisk to the type of the parameter. 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@92 PS12, Line 92: s what it was any other error but IsAlreadyPresent? http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/index_skipscan-test.cc@270 PS12, Line 270: int schema_num = static_cast<int>(GetParam()); Does it make sense to add additional dimension to the test: whether the skip-scan feature enabled? In that way we can make sure that the predicates return the same result set in both cases, and that seems to be a good sanity check. For an example of combining multiple parameters in a Cartesian product see various tests containing text 'testing::Combine' http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/index_skipscan-test.cc@280 PS12, Line 280: ASSERT_NO_FATAL_FAILURE nit: you could use NO_FATALS() shortcut instead for better readability http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/index_skipscan-test.cc@321 PS12, Line 321: case kThreePK: { : // Test predicate on the third PK column. : ScanSpec spec; : Slice value_p3("2_p3"); : auto pred_p3 = ColumnPredicate::Equality(schema_.column(2), &value_p3); : spec.AddPredicate(pred_p3); : vector<string> results; : ASSERT_NO_FATAL_FAILURE(ScanTablet(&spec, &results, "Exact match on P3")); : EXPECT_EQ(20, results.size()); : break; : } nit: use the same layout of scopes and braces as in other cases (that's for better readability). -- 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: 12 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: Wed, 08 Aug 2018 04:13:35 +0000 Gerrit-HasComments: Yes
