Todd Lipcon has posted comments on this change.

Change subject: KUDU-1398 CFile index blocks can store shortest separating 
prefix
......................................................................


Patch Set 8:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/3304/8/src/kudu/cfile/binary_dict_block.cc
File src/kudu/cfile/binary_dict_block.cc:

Line 176:     
data_builder_->GetLastKey(reinterpret_cast<void*>(&last_codeword));
RETURN_NOT_OK here


http://gerrit.cloudera.org:8080/#/c/3304/8/src/kudu/cfile/binary_plain_block.h
File src/kudu/cfile/binary_plain_block.h:

Line 62:   // key should be a Slice *
'key_void' should be a Slice*


PS8, Line 63: void *k
style: void* (no space)


PS8, Line 63: size_t
better to use 'int' (we try to avoid unsigned types these days)


Line 71:   Status GetLastKey(void *key) const OVERRIDE;
same


http://gerrit.cloudera.org:8080/#/c/3304/8/src/kudu/cfile/cfile_util.h
File src/kudu/cfile/cfile_util.h:

Line 109: void GetSeparatingKey(const Slice& left, Slice& right);
should be Slice* (we avoid non-const refs)


http://gerrit.cloudera.org:8080/#/c/3304/8/src/kudu/cfile/cfile_writer.cc
File src/kudu/cfile/cfile_writer.cc:

Line 141:     last_key_.clear();
the default is empty, so no need to clear


http://gerrit.cloudera.org:8080/#/c/3304/8/src/kudu/cfile/rle_block.h
File src/kudu/cfile/rle_block.h:

Line 269:     return Status::NotFound("No keys in the block");
usually we put the error case first, eg:

if (PREDICT_FALSE(count_ == 0)) {
  return Status::NotFound(...);
}

(the philosophy is to keep the 'straight line' or 'common' code path with as 
little indentation as possible)


http://gerrit.cloudera.org:8080/#/c/3304/8/src/kudu/tablet/deltafile.cc
File src/kudu/tablet/deltafile.cc:

Line 76:   opts.optimize_index_keys = false;
would be good to include a comment here explaining why (it's in the commit 
message, but people won't see that when reading the code)


-- 
To view, visit http://gerrit.cloudera.org:8080/3304
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I68ae9146fabd4a19b17d103d118d2d60e28bb315
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wdberke...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com>
Gerrit-HasComments: Yes

Reply via email to