Andrew Wong has posted comments on this change. Change subject: Predicate evaluation pushdown ......................................................................
Patch Set 7: (11 comments) http://gerrit.cloudera.org:8080/#/c/3990/7/src/kudu/cfile/binary_prefix_block.h File src/kudu/cfile/binary_prefix_block.h: Line 99: cur_idx_ += *n; > surprised this doesn't need to update cur_val_ and next_ptr_ Done, realized there should also be a way to do this in the base class using the public methods. http://gerrit.cloudera.org:8080/#/c/3990/7/src/kudu/cfile/rle_block.h File src/kudu/cfile/rle_block.h: Line 190: void SeekForward(size_t *n) override { > hrm, I'm skeptical of this function, I'd think it would need to update the Done Line 413: void SeekForward(size_t *n) override { > same Done http://gerrit.cloudera.org:8080/#/c/3990/7/src/kudu/common/column_eval_context.h File src/kudu/common/column_eval_context.h: Line 26: // get passed down to the decoders during predicate push-down. > this is now more generally the set of objects passed down to the column ite Done Line 31: public: > nit: indent one Done Line 51: // I.e. BinaryDictBlockDecoder.CopyNextAndEval(), CFileIterator.Scan() > not quite following this comment. when is this used outside of scans? Right. Now that Seeks no longer depend on a context, I don't think there will be cases where the block is null. If the this is the case, we should be able to set kDecoderEvalNotSupported in constructor and not have to worry about it later on. Line 54: // Selection vector reflecting the result of the predicate evaluation. > should clarify whether this is an "out" or an "in-out" type parameter. It is an "in-out" parameter. Done Line 63: // materializing_iterator_decoder_eval to false, forcing evaluation to fall > I think this comment needs an update I changed this to explicitly specify the expected use cases. Is there something else that is unclear? Line 74: if (decoder_eval_status_ == kNotSet) { > it seems weird that this function is a silent no-op in the case that it's a Presumably, this is being called by the decoder. If kept as a no-op, I don't think results would change: 1) Start out with kDecoderEvalNotSupported. 2) Materialize some rows without marking them in any selection vector. 3) Set kDecoderEvalSupported, which will no-op. 4) Materialization continues down the "not supported" path. I agree that for SetDecoderEvalNotSupported it makes a larger difference, but here, I think a DCHECK will suffice. http://gerrit.cloudera.org:8080/#/c/3990/7/src/kudu/common/generic_iterators.cc File src/kudu/common/generic_iterators.cc: Line 26: #include "kudu/common/column_eval_context.h" > nit: sort the includes Done http://gerrit.cloudera.org:8080/#/c/3990/7/src/kudu/common/types.h File src/kudu/common/types.h: Line 399 > nit: removed a blank line here 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: 7 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
