Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9372 )
Change subject: rowset_metadata: cache min/max encoded keys ...................................................................... Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/9372/7/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/9372/7/src/kudu/tablet/cfile_set.cc@154 PS7, Line 154: rowset_metadata_->min_encoded_key().empty() && : !rowset_metadata_->max_encoded_key().empty() instead of checking for .empty(), should we make these optional? an empty encoded key is valid in the case that you have a string column and insert "". Granted, you'll only have one of them, but still seems it would be more correct to base it on whether we have the data, not whether it's an empty string http://gerrit.cloudera.org:8080/#/c/9372/7/src/kudu/tablet/diskrowset.cc File src/kudu/tablet/diskrowset.cc: http://gerrit.cloudera.org:8080/#/c/9372/7/src/kudu/tablet/diskrowset.cc@80 PS7, Line 80: TAG_FLAG(rowset_metadata_store_keys, advanced); let's tag as experimental for now as well, since we may well make this default and remove the flag at some point http://gerrit.cloudera.org:8080/#/c/9372/7/src/kudu/tablet/rowset_metadata.h File src/kudu/tablet/rowset_metadata.h: http://gerrit.cloudera.org:8080/#/c/9372/7/src/kudu/tablet/rowset_metadata.h@244 PS7, Line 244: std::string min_encoded_key_; per comment elsewhere, these probably should be optional<> http://gerrit.cloudera.org:8080/#/c/9372/7/src/kudu/tablet/rowset_metadata.cc File src/kudu/tablet/rowset_metadata.cc: http://gerrit.cloudera.org:8080/#/c/9372/7/src/kudu/tablet/rowset_metadata.cc@83 PS7, Line 83: min_encoded_key_ = pb.min_encoded_key(); I think we need to set them to 'none' in the else case -- it seems all of the other code explicitly "zeros" the fields so that you could call LoadFromPB() twice with different PBs and not have leftover data from the prior call http://gerrit.cloudera.org:8080/#/c/9372/7/src/kudu/tablet/rowset_metadata.cc@164 PS7, Line 164: mutable_min_encoded_key()->assign is this different than just using set_mutable_min_encoded_key(min_encoded_key)? -- To view, visit http://gerrit.cloudera.org:8080/9372 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I37d6f7160e3a7188753684e063963110f70e9b8d Gerrit-Change-Number: 9372 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 04 Apr 2018 00:30:40 +0000 Gerrit-HasComments: Yes