Todd Lipcon has posted comments on this change.

Change subject: Predicate evaluation pushdown

Patch Set 12:


mostly just some remaining nits, though I think I spotted one real bug too.
File src/kudu/cfile/

Line 192:       codewords_matching_pred_(nullptr),
hrm, this member variable doesn't seem to be used

PS12, Line 269:  data_block_

Line 293:       continue;
wondering why 'out++' doesn't need to be called in this case. I guess we are 
lacking some test coverage of the multi-predicate case? maybe move 'out++' into 
the for loop? (eg 'i++, out++')
File src/kudu/cfile/

Line 288:   for (i = 0; i < max_fetch; i++) {
'i' can be deifned on the line above now
File src/kudu/cfile/binary_prefix_block.h:

Line 20: #include <algorithm>
why's this needed?
File src/kudu/cfile/

Line 701:   if (!dict_decoder_ && reader_->footer().has_dict_block_ptr()) {
nit: seem to have lost a useful comment here

PS12, Line 900: l = SelectionVectorView(c
nit: remaining_sel(ctx->sel())

PS12, Line 942: / TODO: Maybe copy all and shift later?
any idea what this comment means? I think I wrote it but I don't remember, so 
perhaps we should just remove it
File src/kudu/cfile/cfile_reader.h:

PS12, Line 451: >c
nit: missing space

Line 470:   ColumnMaterializationContext* ctx_;
this is no longer used, right?
File src/kudu/cfile/gvint_block.h:

Line 22: #include <algorithm>
File src/kudu/common/column_materialization_context.h:

PS12, Line 33: ializationContext(size_t col_idx,
             :                     const ColumnPredicate* pred,
             :                     ColumnBlock* block,
             :                     S
nit: indentation got messed up with the rename

PS12, Line 42:       if (!pred_ || !sel || !block) {
             :         decoder_eval_status_ = kDecoderEvalNotSupported;
nit: should be indented -2 from here
File src/kudu/common/rowblock.h:

PS12, Line 127:  size_t first_row_idx = 0
is this optional parameter used? maybe I missed it
File src/kudu/tablet/

Line 412:     RETURN_NOT_OK(col_iter->SeekToOrdinal(cur_idx_));
lost a useful comment above here (in a few places)

Line 427:   return Status::OK();
other spurious removal of blank lines above in this function
File src/kudu/tablet/

Line 122: string DeltaIteratorMerger::ToString() const {
nit: add blank line above
File src/kudu/tablet/deltamemstore.h:

Line 207:  private:
nit: blank line above

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I31e4cce21e99f63b089d7c84410af8ed914cb576
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to