Andrew Wong has posted comments on this change. Change subject: Predicate evaluation pushdown ......................................................................
Patch Set 12: (18 comments) 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 Ah, right. Removed. PS12, Line 269: data_block_ > data_decoder_ Done Line 293: continue; > wondering why 'out++' doesn't need to be called in this case. I guess we ar Right, so in a multi-predicate query, this would lead to strings being materialized to the wrong spot, provided the previous column had non-zero selectivity. Will write up a test case. 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 Done 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? SeekForward was defined here first, which used min. Not needed anymore. 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 Done PS12, Line 900: l = SelectionVectorView(c > nit: remaining_sel(ctx->sel()) Done 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, Think it might've been to materialize the entire column in one "batch" and do batched (shifted) evaluation after? Not sure. Regardless, yea I'll 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 Done Line 470: ColumnMaterializationContext* ctx_; > this is no longer used, right? Done 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 Done 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 Done PS12, Line 42: if (!pred_ || !sel || !block) { : decoder_eval_status_ = kDecoderEvalNotSupported; : > nit: should be indented -2 from here Done 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 Not used at the moment. Thought it might be useful to have, but it can be taken out. 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) Done Line 427: return Status::OK(); > other spurious removal of blank lines above in this function Done 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 Done 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 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: 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
