Andrew Wong has posted comments on this change.

Change subject: Predicate evaluation pushdown

Patch Set 7:

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.
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 

Line 413:   void SeekForward(size_t *n) override {
> same
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

Line 31: public:
> nit: indent one

Line 51:   // I.e. BinaryDictBlockDecoder.CopyNextAndEval(), 
> 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 
> 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.
File src/kudu/common/

Line 26: #include "kudu/common/column_eval_context.h"
> nit: sort the includes
File src/kudu/common/types.h:

Line 399
> nit: removed a blank line here

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I31e4cce21e99f63b089d7c84410af8ed914cb576
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to