Andrew Wong has posted comments on this change. Change subject: Predicate evaluation pushdown ......................................................................
Patch Set 10: (8 comments) http://gerrit.cloudera.org:8080/#/c/3990/10/src/kudu/cfile/cfile-test-base.h File src/kudu/cfile/cfile-test-base.h: Line 309: return ctx; > can just be 'return ColumnMaterializationContext(0, nullptr, cb, sel);' Done http://gerrit.cloudera.org:8080/#/c/3990/10/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: 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 CFileIterator. 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 thing. http://gerrit.cloudera.org:8080/#/c/3990/10/src/kudu/cfile/cfile_reader.h 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). http://gerrit.cloudera.org:8080/#/c/3990/10/src/kudu/cfile/encoding-test.cc File src/kudu/cfile/encoding-test.cc: PS10, Line 540: i*skip_step+skip > nit: i * skip_step + skip (spaces between operators) Done http://gerrit.cloudera.org:8080/#/c/3990/10/src/kudu/cfile/rle_block.h 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. http://gerrit.cloudera.org:8080/#/c/3990/10/src/kudu/tablet/tablet-decoder-eval-test.cc File src/kudu/tablet/tablet-decoder-eval-test.cc: PS10, Line 137: StringPrintf(Substitute("%0$0$1", strlen, PRId64).c_str(), > this cleverness repeats several times, worth factoring into a function like Done Line 224: int main(int argc, char *argv[]) { > shouldn't need a main -- it shoudl get there automagically from the kudu_te Done -- To view, visit http://gerrit.cloudera.org:8080/3990 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I31e4cce21e99f63b089d7c84410af8ed914cb576 Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <andrew.w...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
