Todd Lipcon has posted comments on this change. Change subject: Predicate evaluation pushdown ......................................................................
Patch Set 12: (18 comments) mostly just some remaining nits, though I think I spotted one real bug too. http://gerrit.cloudera.org:8080/#/c/3990/12/src/kudu/cfile/binary_dict_block.cc File src/kudu/cfile/binary_dict_block.cc: Line 192: codewords_matching_pred_(nullptr), hrm, this member variable doesn't seem to be used 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 are lacking some test coverage of the multi-predicate case? maybe move 'out++' into the for loop? (eg 'i++, out++') http://gerrit.cloudera.org:8080/#/c/3990/12/src/kudu/cfile/binary_plain_block.cc File src/kudu/cfile/binary_plain_block.cc: Line 288: for (i = 0; i < max_fetch; i++) { 'i' can be deifned on the line above now http://gerrit.cloudera.org:8080/#/c/3990/12/src/kudu/cfile/binary_prefix_block.h File src/kudu/cfile/binary_prefix_block.h: Line 20: #include <algorithm> why's this needed? http://gerrit.cloudera.org:8080/#/c/3990/12/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: 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, so perhaps we should just remove it http://gerrit.cloudera.org:8080/#/c/3990/12/src/kudu/cfile/cfile_reader.h File src/kudu/cfile/cfile_reader.h: PS12, Line 451: >c nit: missing space Line 470: ColumnMaterializationContext* ctx_; this is no longer used, right? http://gerrit.cloudera.org:8080/#/c/3990/12/src/kudu/cfile/gvint_block.h File src/kudu/cfile/gvint_block.h: Line 22: #include <algorithm> unused http://gerrit.cloudera.org:8080/#/c/3990/12/src/kudu/common/column_materialization_context.h 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 http://gerrit.cloudera.org:8080/#/c/3990/12/src/kudu/common/rowblock.h File src/kudu/common/rowblock.h: PS12, Line 127: size_t first_row_idx = 0 is this optional parameter used? maybe I missed it http://gerrit.cloudera.org:8080/#/c/3990/12/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: 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 http://gerrit.cloudera.org:8080/#/c/3990/12/src/kudu/tablet/delta_iterator_merger.cc File src/kudu/tablet/delta_iterator_merger.cc: Line 122: string DeltaIteratorMerger::ToString() const { nit: add blank line above http://gerrit.cloudera.org:8080/#/c/3990/12/src/kudu/tablet/deltamemstore.h File src/kudu/tablet/deltamemstore.h: Line 207: private: nit: blank line above -- 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: 12 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
