Andrew Wong 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 PrepareMateria
This should work, and it would clean up the API for when a context is needed. 
Just need to make sure there's a clean way to pass it to the decoder from the 

PS10, Line 976: !ctx->DecoderEvalNotSupported
> it strikes me that most of the calls are double-negatives. Worth adding a D
A few of these should be !NotSupported, since they should act either when the 
state is Supported or NotSet (e.g. we should CopyNextAndEval if the state is 
NotSet or Supported).
Here, the decoder may not have made a call to the one of the Copy* functions 
(e.g. if we're starting out with a null run), in which case, we would be NotSet 
rather than NotSupported.
We could have a DecoderEvalMayBeSupported, which would effectively be the same 
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 
Yep, more explicitly, this gets used when determining the matching codewords 
during scan preparation. Pushing codeword matching to right before a scan 
should alleviate this, since we would only require the ctx when we really need 
it (i.e. right before/during a scan).
File src/kudu/cfile/

PS10, Line 540: i*skip_step+skip
> nit: i * skip_step + skip (spaces between operators)
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?
No, removed.
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

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

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