Will Berkeley has posted comments on this change.

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

Patch Set 4:


Commit Message:

PS4, Line 17: compatibility with bloomfile
> what's this mean?
There was a bug making bloom files not work. The trick is that bloom files call 
AppendRawBlock directly instead of FinishCurDataBlock. I altered AppendRawBlock 
to accept the last key as well as the first key, so now a BloomFileWriter 
maintains its own last key, just like it maintains its own first key. I feel 
like the change is a little awkward though, so I'd appreciate any ideas on how 
to improve it.

File src/kudu/cfile/binary_dict_block.cc:

Line 131:     last_key_.assign_copy(last->data(), last->size());
> instead of copying this every time, could we use GetLast on the code word b
Doesn't the dictionary in the block builder map from string to codeword? The 
last key we want is the last string in the block, not the codeword stored for 
it, so we can't look it up in the dictionary. I did improve this, though, by 
looking up in the dict block via GetKeyAtIdx, a method which was added to plain 
encoding per one of your comments. This means I had to drop CHECK(finished_) 
from it (but kept it in GetFirstKey and GetLastKey). Does that solution seem 
reasonable to you?

File src/kudu/cfile/binary_plain_block.cc:

Line 132: Status BinaryPlainBlockBuilder::GetLastKey(void *key_void) const {
> can we refactor this to share code with GetFirstKey? eg a GetKeyAtIndex(int

File src/kudu/cfile/cfile_util.cc:

Line 126: void SeparatingKey(const faststring& left, const faststring& right, 
faststring *target) {
> can you add some unit tests for this function? (eg the examples from the co

Line 127:   DCHECK(memcmp(left.data(), right.data(), min(left.size(), 
right.size())) <=
> I think DCHECK_LE(Slice(left).compare(Slice(right), 0); is probably more re

Line 130:   if (left.size() == 0) {
> .empty()

Line 131:     // special case for first block: use the whole key
> not sure why this is the case
Bloomfiles work with this now; no more special case needed.

Line 135:     target->assign_copy(right.data(), cpl == right.size() ? cpl : cpl 
+ 1);
> hm, this is a slightly different algorithm than the one used in gutil/strin
I didn't realize that function existed! Still, it finds a separator in the 
range left <= * < right, and we require it in the range left < * <= right. I 
thought about it a bit and I don't see any advantage.

File src/kudu/cfile/cfile_util.h:

Line 63:   bool deltafile;
> I think I'd prefer this be called something like 'optimize_index_keys' or s
I'm going to keep it simple for now and just rename, but I'll consider 
implementing your suggestion for delta keys a little later.

Line 110: // Computes the shortest key satisfying left < key < right and places 
it into target.
> should clarify whether 'target' may be the same faststring as 'left' or 'ri
Superseded by another change.

PS4, Line 111: const faststring& left, const faststring& right
> may make sense to use StringPiece or Slice here rather than specifically fa
Agreed. Converted to use Slice.

Line 111: void SeparatingKey(const faststring& left, const faststring& right, 
faststring *target);
> rename to GetSeparatingKey

File src/kudu/cfile/cfile_writer.cc:

Line 141:     last_key_.clear();
> no need (it's initialized to clear)

Line 437:       SeparatingKey(last_key_, tmp_buf_, &sep_buf);
> maybe make SeparatingKey actually just shorten tmp_buf_ in place to avoid t

File src/kudu/cfile/rle_block.h:

Line 265:     if (count_ > 0) {
> usually we put the error case first
I'm being consistent with GetFirstKey's (preexisting) implementation. If you 
like, I'll adjust both of them to handle error case first.

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: 4
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