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 15: (7 comments) I have addressed all the open comments. PTAL. http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/cfile/cfile_reader.h File src/kudu/cfile/cfile_reader.h: http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/cfile/cfile_reader.h@59 PS10, Line 59: > Yep, it would be nice to have a test with super-wide primary key (multiple Added a test case where the encoded primary key size is equal to the max allowable encoded PK size (16MB), stored as max_encoded_key_size_bytes. Also, verified that any insertions that violate this size constraint results in failure. 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@444 PS12, Line 444: col_id = > If that's something we expect to happen, I would simply disable the skip sc Done 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; : : > I think it's better to keep it as Andrew requested. After some considerati Sure. Done. 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: id CFileSet::Ite > nit: since this is not to change in the code below, add 'const' Done 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 Done http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/tablet/cfile_set.cc@699 PS14, Line 699: : skip_scan_num_seeks_++ > I think it makes sense to introduce a metric to capture cases when skip sca Added this info in VLOG(1). http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/tablet/index_skipscan-test.cc File src/kudu/tablet/index_skipscan-test.cc: http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/tablet/index_skipscan-test.cc@189 PS14, Line 189: break; : } : case kMaxSizedEncodedKey: { : CHECK_OK(builder.AddKeyColumn("P1", STRING)); : CHECK_OK(builder.AddKeyColumn > nit: all there might be constexpr: Done -- 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: 15 Gerrit-Owner: Anupama Gupta <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[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: Wed, 15 Aug 2018 04:55:52 +0000 Gerrit-HasComments: Yes
