Todd Lipcon has posted comments on this change.

Change subject: Predicate evaluation pushdown

Patch Set 10:

File src/kudu/cfile/cfile-test-base.h:

Line 309:     return ctx;
can just be 'return ColumnMaterializationContext(0, nullptr, cb, sel);'
File src/kudu/cfile/

PS10, Line 719: 
              :     // ctx_ should only be null if 
PrepareMaterializationContext() is not called, in which
              :     // case only seeks are permitted.
              :     if (ctx_ && !ctx_->DecoderEvalNotSupported()) {
could you get rid of the ctx_ member variable along with the 
PrepareMaterializationContext() call by moving this whole block into the Scan 
method itself as a lazy initialization? eg something like:

if (dict_decoder_ && !ctx->DecoderEvalNotSupported() && 
!codewords_matching_pred_) {
  // ... initialize codewords_matching_pred_

PS10, Line 976: !ctx->DecoderEvalNotSupported
it strikes me that most of the calls are double-negatives. Worth adding a 
DecoderEvalSupported() getter?
File src/kudu/cfile/cfile_reader.h:

PS10, Line 230: used during a scan.
it's a bit confusing here, since this seems to be saying you set it so you can 
use it during a scan, but then you are passing one to Scan() as well.
File src/kudu/cfile/

PS10, Line 540: i*skip_step+skip
nit: i * skip_step + skip (spaces between operators)

(same above on line 536 and 532)
File src/kudu/cfile/rle_block.h:

Line 190:   void SeekForward(int* n) override {
is this still necessary now that you have the superclass implementation?
File src/kudu/tablet/

PS10, Line 137:  StringPrintf(Substitute("%0$0$1", strlen, PRId64).c_str(),
this cleverness repeats several times, worth factoring into a function like 
LeftZeroPad(int n, int strlen) or somesuch

Line 224: int main(int argc, char *argv[]) {
shouldn't need a main -- it shoudl get there automagically from the 
kudu_test_main linked into all the tests

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I31e4cce21e99f63b089d7c84410af8ed914cb576
Gerrit-PatchSet: 10
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