Alexey Serbin has posted comments on this change.

Change subject: Predicate evaluation pushdown

Patch Set 3:


First pass -- I just saw a new version has appeared.  Will continue with the 
new version.
Commit Message:

Line 16:
Nit: it would be nice to have the document in the Kudu source tree when it's 
time to submit the changes.  I.e., please add that .md file into the 
changelist.  Probably, $REPO_ROOT/docs/design-docs is the right place to add 
the file.  Don't forget to update the comment once it's time to push the change 
into the central repo.  Better is to put the correct, relative link here, and 
add the temporary location of the documents into some sort of gerrit-specific 

Line 20:
File src/kudu/benchmarks/tpch/

Line 87: DEFINE_string(tpch_path_to_dbgen_dir, "/Users/andrew.wong/tpch-dbgen",
This does not look as a good default.  I think ".", "./tpch" or "./tpch-dbgen" 
would be better.
File src/kudu/cfile/

Line 252: // store their min/max values. CopyNextAndEval in these blocks could
Ditto for the asterisk (star) as for the corresponding header file.

Line 289:       // Row is included in predicate, copy data to block.
nit: if I'm not mistaken, the style guide mentions if/else should be formatted 

if (...) {
} else {
File src/kudu/cfile/

Line 191:   dict_decoder_ = iter->GetDictDecoder();
Why not to move those into the initializer list?
File src/kudu/cfile/binary_dict_block.h:

Line 131:   Status CopyNextAndEval(size_t *n,
Nit: it seems in the majority of the code in this file the asterisk tends to be 
with parameter type, not parameter name. It would be nice to have the same for 
the newly added members/methods/functions.
File src/kudu/cfile/binary_dict_block.h:

Line 131:   Status CopyNextAndEval(size_t *n,
Style nit: the rest of this file uses the "asterisk is closer to the type" 
convention.  Consider following it here as well, if it makes sense.
File src/kudu/cfile/

Line 290:   size_t i;
If the 'i' variable is not needed outside of the cycle, why not to move it 
inside the cycle's visibility, like

for (size_t i = 0; ...)
File src/kudu/cfile/binary_plain_block.h:

Line 110: 
What if SeekForward returned OK only if it moved forward at the specified 
number of bytes and returned Status::Incomplete() otherwise?
File src/kudu/cfile/binary_plain_block.h:

Line 111:   virtual Status SeekForward(size_t* n) OVERRIDE {
Does it make sense to implement this method in the base class instead?

Line 112:     DCHECK(HasNext());
Why not to return non-OK status in that case instead?  I.e., always assign the 
actual number of positions it was forwarded into the output parameter and 
return non-OK status when it's not possible to seek forward to the specified 
number of positions.
File src/kudu/cfile/block_encodings.h:

Line 162:     CopyNextValues(n, dst);
What if CopyNextValues() returned non-OK code?  Would it make sense to check 
for that and return right away in that case?
File src/kudu/cfile/bshuf_block.h:

Line 278:   Status SeekForward(size_t* n) OVERRIDE {
Consider returning non-OK status code (like Status::Incomplete()) instead of 
DCHECK()) if it's not possible to advance current index as far as desired.
File src/kudu/cfile/plain_block.h:

Line 211:   virtual Status SeekForward(size_t* n) OVERRIDE {
Does it make sense to implement this method in the base class instead?
File src/kudu/common/rowblock.h:

Line 123:   explicit SelectionVectorView(SelectionVector *sel_vec, size_t 
first_row_idx = 0)
It's not clear why it would be necessary to have this wrapper for nullptr 
SelectionVector.  Could you add a comment to clarify this?

Line 125:     if (sel_vec) {
Nit: the check for sel_vec_ is already performed by the Advance() method.

Line 133:     row_offset_ += skip;
Why to advance the row_offset_ if there is no underlying data?  Please add a 
comment about the reason behind this.

Line 146:   if (sel_vec_) {
Nit: tab/shift is missing
File src/kudu/common/types.h:

PS3, Line 59: std::function<int(const void*, const void*)>
Consider introducing typedef for this functor.
File src/kudu/tablet/

Line 453:     LOG(WARNING) << "Unable to prepare column " << ctx->col_idx() << 
": " << s.ToString();
nit: why WARNING, not ERROR?
File src/kudu/tablet/

Line 474: 
Why is it necessary to have this alias?

To view, visit
To unsubscribe, visit

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

Reply via email to