Alexey Serbin has posted comments on this change.

Change subject: KUDU-1363: Add IN-list predicate type
......................................................................


Patch Set 16:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/2986/16//COMMIT_MSG
Commit Message:

PS16, Line 9: clients
'C++ clients'  ?


http://gerrit.cloudera.org:8080/#/c/2986/16/src/kudu/client/client.cc
File src/kudu/client/client.cc:

PS16, Line 726: std::
nit: the file already states 'using std::vector', so this prefix might be 
dropped.


http://gerrit.cloudera.org:8080/#/c/2986/16/src/kudu/client/client.h
File src/kudu/client/client.h:

PS16, Line 848: values list
nit: 'values container itself and its elements' ?


http://gerrit.cloudera.org:8080/#/c/2986/16/src/kudu/common/column_predicate-test.cc
File src/kudu/common/column_predicate-test.cc:

Line 367:     top_list = {&values[1], &values[3], &values[6]};
Consider also adding test cases for

| |
  | |

and 

  | |
| |

(actually, I think the second is already covered by this one, but just in case).

Also, what about a test cases for the following pairs?

| |
 | |

 | |
| |

and

| | |
 | |

 | |
| | |


http://gerrit.cloudera.org:8080/#/c/2986/16/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:

Line 381:       // Remove all values in this IN list which are greater than the 
largest
Does it make sense to check that other.values_ is not empty before proceeding?


Line 393:       if (values_.empty()) return;
Does it make sense to call SetToNone() before return?


Line 410:       values_.erase(std::remove_if(values_.begin(), values_.end(), 
other_absent), values_.end());
Before returning from this method, does it make sense to check and call 
SetToNone() if the values_ container became empty?


http://gerrit.cloudera.org:8080/#/c/2986/16/src/kudu/common/key_util.cc
File src/kudu/common/key_util.cc:

PS16, Line 179: predicate->raw_values().back()
What if the IN list was empty?  Is that possible that it gets down to this 
point?  If yes, consider checking for that condition before trying to access 
the last element in the IN list.


PS16, Line 242: predicate->raw_values().front()
Ditto, but for the first element of the IN list.


http://gerrit.cloudera.org:8080/#/c/2986/16/src/kudu/common/scan_spec-test.cc
File src/kudu/common/scan_spec-test.cc:

PS16, Line 74: vector<T> values
const vector<T>& ?


http://gerrit.cloudera.org:8080/#/c/2986/16/src/kudu/common/scan_spec.cc
File src/kudu/common/scan_spec.cc:

PS16, Line 153: schema.column(col_idx).name()
nit: it's not in the code you modified, but if you have some time, consider 
replacing with the dedicated variable 'column'.


http://gerrit.cloudera.org:8080/#/c/2986/16/src/kudu/common/wire_protocol-test.cc
File src/kudu/common/wire_protocol-test.cc:

Line 339: TEST_F(WireProtocolTest, TestColumnPredicateInList) {
Does it make sense to add a test to check for some "error" scenarios, like an 
empty IN list?  I saw that case is handled in the code, so if we are 
considering an empty IN list as a valid case, consider making it explicit via 
adding a test.

Also, if it makes any sense, consider adding a test for duplicates in the IN 
list, verifying that the result predicate sent to the server does not contain 
duplicates.  I saw the duplicates are handled in the IN-list's code, so I 
thought it might make sense adding some coverage for that.


PS16, Line 346: 3
Nit: wouldn't ARRAYSIZE() be more elegant instead?


-- 
To view, visit http://gerrit.cloudera.org:8080/2986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar <abhyan...@gmail.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar <abhyan...@gmail.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to