[kudu-CR] rowset metadata: cache min/max encoded keys

["Todd Lipcon (Code Review)" <ge...@cloudera.org>]

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 <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 04 Apr 2018 00:30:40 +0000
Gerrit-HasComments: Yes