Andrew Wong has posted comments on this change.

Change subject: Predicate evaluation pushdown

Patch Set 12:

File src/kudu/cfile/

Line 192:       codewords_matching_pred_(nullptr),
> hrm, this member variable doesn't seem to be used
Ah, right. Removed.

PS12, Line 269:  data_block_
> data_decoder_

Line 293:       continue;
> wondering why 'out++' doesn't need to be called in this case. I guess we ar
Right, so in a multi-predicate query, this would lead to strings being 
materialized to the wrong spot, provided the previous column had non-zero 
selectivity. Will write up a test case.
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?
SeekForward was defined here first, which used min. Not needed anymore.
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, 
Think it might've been to materialize the entire column in one "batch" and do 
batched (shifted) evaluation after? Not sure. Regardless, yea I'll 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>
> unused
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
Not used at the moment. Thought it might be useful to have, but it can be taken 
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