Todd Lipcon 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);' 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 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? 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 can use it during a scan, but then you are passing one to Scan() as well. 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) (same above on line 536 and 532) 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? 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 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 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
