Alexey Serbin has posted comments on this change.

Change subject: Predicate evaluation pushdown
......................................................................


Patch Set 4:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/3990/4/src/kudu/tablet/deltamemstore.cc
File src/kudu/tablet/deltamemstore.cc:

Line 368:     if (col.size() > 0) {
Consider using empty() method instead.


Line 372:   return deletes_and_reinserts_.size() > 0;
Consider using empty() method instead of size().

It also might make sense to re-order those checks: first check for 
deletes_and_reinserts and then updated_by_col_.


http://gerrit.cloudera.org:8080/#/c/3990/4/src/kudu/tablet/tablet-decoder-eval-test.cc
File src/kudu/tablet/tablet-decoder-eval-test.cc:

PS4, Line 92: fetched
Is this passed by a reference and then modified by the function?  If yes, then 
it does not follow the code style guide -- it should be passed by pointer 
instead: https://google.github.io/styleguide/cppguide.html#Reference_Arguments


PS4, Line 100: TEST_P(TabletDecoderEvalTest, LowCardinality) {
             :   FillTestTablet(50);
             :   for (int i = 0; i < FLAGS_decoder_eval_test_nrepeats; i++) {
             :     TestTimedScanAndFilter(50);
             :   }
             : }
             : 
             : TEST_P(TabletDecoderEvalTest, MidCardinality) {
             :   FillTestTablet(1000);
             :   for (int i = 0; i < FLAGS_decoder_eval_test_nrepeats; i++) {
             :     TestTimedScanAndFilter(1000);
             :   }
             : }
             : 
             : TEST_P(TabletDecoderEvalTest, HighCardinality) {
             :   FillTestTablet(50000);
             :   for (int i = 0; i < FLAGS_decoder_eval_test_nrepeats; i++) {
             :     TestTimedScanAndFilter(50000);
             :   }
             : }
Is it possible to re-factor this separating the common code into a class method 
and then call that method with parameters 50, 1000, 50000 instead of 
copying-pasting?


http://gerrit.cloudera.org:8080/#/c/3990/4/src/kudu/tablet/tablet-test-util.h
File src/kudu/tablet/tablet-test-util.h:

PS4, Line 144: inline
Why is it necessary to have this function in-lined?  Just because there isn't 
corresponding *.cc file?


PS4, Line 145: int& fetched
Should be passed by pointer, if following google's style guide: 
https://google.github.io/styleguide/cppguide.html#Reference_Arguments


-- 
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <andrew.w...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to