[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-28 Thread Dan Burkert (Code Review) 
Dan Burkert has submitted this change and it was merged.

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


KUDU-1363: Add IN-list predicate type

Adds support in the C++ client for providing a set of equalities for a
given column. Support for using IN list predicates from the Java client
will be in a follow-up commit.

Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
Reviewed-on: http://gerrit.cloudera.org:8080/2986
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin 
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/scan_predicate.h
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol-test.cc
M src/kudu/common/wire_protocol.cc
16 files changed, 1,000 insertions(+), 37 deletions(-)

Approvals:
  Alexey Serbin: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
Gerrit-PatchSet: 20
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-28 Thread Alexey Serbin (Code Review) 
Alexey Serbin has posted comments on this change.

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


Patch Set 19: Code-Review+2

-- 
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: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-28 Thread Dan Burkert (Code Review) 
Hello Adar Dembo, Alexey Serbin, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/2986

to look at the new patch set (#19).

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

KUDU-1363: Add IN-list predicate type

Adds support in the C++ client for providing a set of equalities for a
given column. Support for using IN list predicates from the Java client
will be in a follow-up commit.

Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/scan_predicate.h
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol-test.cc
M src/kudu/common/wire_protocol.cc
16 files changed, 1,000 insertions(+), 37 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/2986/19
-- 
To view, visit http://gerrit.cloudera.org:8080/2986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
Gerrit-PatchSet: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-28 Thread Dan Burkert (Code Review) 
Dan Burkert has posted comments on this change.

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


Patch Set 18:

(3 comments)

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

Line 464: // = |  |
> nit: is this supposed to be 
Done


Line 472: // <) AND
> Just in case, consider adding tests for boundary conditions like
The equality ones are already tested in the equality + range section.  I've 
added some additional tests to make sure single element lists get simplified to 
an equality.   Added the rest.


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

PS18, Line 357: ASSERT_EQ
> Here and below for the ASSERT_EQ() macro:
Done


-- 
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: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-28 Thread Alexey Serbin (Code Review) 
Alexey Serbin has posted comments on this change.

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


Patch Set 18:

(3 comments)

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

Line 464: // = |  |
nit: is this supposed to be 

[--->
  | | |
=   | |

?

I hope you are reading this comment in a monospace font :)


Line 472: // <) AND
Just in case, consider adding tests for boundary conditions like

[--) AND
   |


[--) AND
  ||


[--) AND
|  |


Also, I didn't notice that the following cases are covered:

  [) AND
|


  [) AND
|  |


<-) AND
|

<-> AND
 |

   MAX_INT
<-) AND
  |

Consider adding those as well.

Besides, a tiny nit: consider re-grouping the test cases so the range-related 
ones come in one block (if, of course, there isn't any other grouping criterion 
in effect here which I might be missing).


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

PS18, Line 357: ASSERT_EQ
Here and below for the ASSERT_EQ() macro:
if I'm not mistaken, the expected value is the first parameter, the actual is 
the second.


-- 
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: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-27 Thread Dan Burkert (Code Review) 
Hello Adar Dembo, Alexey Serbin, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/2986

to look at the new patch set (#18).

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

KUDU-1363: Add IN-list predicate type

Adds support in the C++ client for providing a set of equalities for a
given column. Support for using IN list predicates from the Java client
will be in a follow-up commit.

Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/scan_predicate.h
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol-test.cc
M src/kudu/common/wire_protocol.cc
16 files changed, 881 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/2986/18
-- 
To view, visit http://gerrit.cloudera.org:8080/2986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-27 Thread Alexey Serbin (Code Review) 
Alexey Serbin has posted comments on this change.

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


Patch Set 17: Code-Review+2

-- 
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: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-27 Thread Dan Burkert (Code Review) 
Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/2986

to look at the new patch set (#17).

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

KUDU-1363: Add IN-list predicate type

Adds support in the C++ client for providing a set of equalities for a
given column. Support for using IN list predicates from the Java client
will be in a follow-up commit.

Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/scan_predicate.h
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol-test.cc
M src/kudu/common/wire_protocol.cc
16 files changed, 880 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/2986/17
-- 
To view, visit http://gerrit.cloudera.org:8080/2986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-27 Thread Dan Burkert (Code Review) 
Dan Burkert has posted comments on this change.

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


Patch Set 17:

(12 comments)

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

PS16, Line 9: he C++ 
> 'C++ clients'  ?
Done


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

PS16, Line 726: char*
> nit: the file already states 'using std::vector', so this prefix might be d
Done


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

PS16, Line 848: p of the va
> nit: 'values container itself and its elements' ?
Done


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 = {[1], [3], [6]};
> Consider also adding test cases for
Done


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

Line 381:   };
> Does it make sense to check that other.values_ is not empty before proceedi
I've added a DCHECK, since it's assumed by this method.


Line 393:  other.values_.front(), 
search_by));
> Does it make sense to call SetToNone() before return?
Yes, great catch.  I've added a test that covers this as well.


Line 410: return this->column_.type_info()->Compare(v, *other_start) != 
0;
> Before returning from this method, does it make sense to check and call Set
I've added a call to Simplify, which does the same thing.  Good catch.  One of 
the new tests you suggested covers this.


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

PS16, Line 179: 
> What if the IN list was empty?  Is that possible that it gets down to this 
We assume an IN list is never empty, but just to be safe i've added a DCHECK 
(since you have already pointed out a few places in this review where that 
could have been violated).


PS16, Line 242: that can be pushed.
> Ditto, but for the first element of the IN list.
Same; added DCHECK.


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: const vector&
> const vector& ?
Done


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

PS16, Line 153: column).predicate_type();
> nit: it's not in the code you modified, but if you have some time, consider
Done


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 
Done


-- 
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: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-27 Thread Alexey Serbin (Code Review) 
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 = {[1], [3], [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 values
const vector& ?


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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-26 Thread Dan Burkert (Code Review) 
Dan Burkert has posted comments on this change.

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


Patch Set 15:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/2986/12/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:

Line 546:   }
> consider adding a test cases for
This actually isn't possible since the KuduValue API has no way to create a 
null value.


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

Line 122:   vals_list.reserve(vals_.size());
> vals_list.reserve(vals_.size()) ?
Done


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

PS12, Line 334: return this->column_.type_info()->Compare(lhs, rhs) < 0;
  :   };
  : 
  :  
> an optimization opportunity if we expect IN() lists to be large.  Get the u
I've added some optimizations around first/last value ranges, let me know if 
you think there is more to do.


Line 352:   return;
> What if it was IN (NULL) predicate?
IN (NULL) is not possible, since KuduValue can't be a null value.


PS12, Line 501: case UINT16: return EvaluateForPhysicalType(block, 
sel);
  : case UINT32: return EvaluateForPhysicalType(block, 
sel);
  : case UINT64: return EvaluateForPhysicalType(block, 
sel);
  : case FLOAT: return EvaluateForPhysicalType(block, 
sel);
  : case DOUBLE: return EvaluateForPhysicalType(block, 
sel);
  : case BINARY: return EvaluateForPhysicalType(block, 
sel);
  : default: LOG(FATAL) << "unknown physical type: " << 
block.type_info()->physical_type();
  :   }
  : }
  : 
  : string ColumnPredicate::ToString() const {
  :   switch (predicate_type()) {
  : case PredicateType::None: return strings::Substitute("`$0` 
NONE", column_.name());
  : case PredicateType::Range: {
  :
> May be, it's time to start using the switch() here instead?
Done


PS12, Line 537: 
> Is this by-copy capture?  Consider replacing with by-reference capture to a
Done


PS12, Line 532: case PredicateType::InList: {
  :   string ss = "`";
  :   ss.append(column_.name());
  :   ss.append("` IN (");
  :   bool is_first = true;
  :   for (auto* value : values_) {
  : if (is_first) {
  :  
> This looks strange to me: given the name of the method, I would expect it t
I've removed this since it was only used in a single place, and I agree that 
the name could be misleading.


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

PS12, Line 154: size_t size = column.type_info()->size();
  : switch (predicate->predicate_type()) {
  :   case PredicateType::Equality:
  : memcpy(row->mutable_cell_ptr(*col_idx_it), 
predicate->raw_lower(), size);
  : pushed_predicates++;
  : final_predicate = predicate;
  : break;
  :   case PredicateType::Range:
  : if (predicate->raw_upper() != nullptr) {
  :   memcpy(row->mutable_cell_ptr(*col_idx_it), 
predicate->raw_upper(), size);
  :   pushed_predicates++;
  :   final_predicate = predicate;
  : }
  : // After the first column with a range constraint we 
stop pushing
  : // constraints into the upper bound. Instead, we push 
minimum values
  : // to the remaining columns (below), which is the 
maximally tight
  : // constraint.
  : break_loop = true;
  : break;
  :   case PredicateType::IsNotNull:
  : break_loop = true;
  : break;
  :   case PredicateType::InList:
  : // Since the InList predicate is a sorted vector of 
values, the last
  :
> nit: consider converting this into switch()
Done in a prequel patch.


PS12, Line 215:   // Step 1: copy predicates into the row in key column order, 
stopping after
  :   // the first missing predicate.
  : 
  :   bool break_loop = false;
  :   for (auto col_idx_it = first; !break_loop && col_idx_it < 
last; std::advance(col_idx_it, 1)) {
  : const ColumnSchema& column = schema.column(*col_idx_it);
  : const ColumnPredicate* predicate = FindOrNull(predicates, 
column.name());
  : if (predicate == nullptr)

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-23 Thread Alexey Serbin (Code Review) 
Alexey Serbin has posted comments on this change.

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


Patch Set 12:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/2986/12/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:

Line 546:   }
consider adding a test cases for
 1. value IN (NULL)
 2. value IN (NULL, )


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

Line 122:   vector vals_list;
vals_list.reserve(vals_.size()) ?


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

PS12, Line 334:   values_.erase(std::remove_if(values_.begin(), 
values_.end(),
  :[] (const void* v){
  :  return 
!other.CheckValueInRange(v);
  :  
an optimization opportunity if we expect IN() lists to be large.  Get the upper 
and lower bounds of range and then found corresponding boundaries in the 
values_.  If the range does not cover the list, copy the values in the range 
into a temporary array and swap with values_.


Line 352: case PredicateType::IsNotNull: return;
What if it was IN (NULL) predicate?


PS12, Line 501:   } else if (predicate_type_ == PredicateType::Equality) {
  : return column_.type_info()->Compare(lower_, other.lower_) 
== 0;
  :   } else if (predicate_type_ == PredicateType::Range) {
  : return (lower_ == other.lower_ ||
  : (lower_ != nullptr && other.lower_ != nullptr &&
  :  column_.type_info()->Compare(lower_, other.lower_) 
== 0)) &&
  :(upper_ == other.upper_ ||
  : (upper_ != nullptr && other.upper_ != nullptr &&
  :  column_.type_info()->Compare(upper_, other.upper_) 
== 0));
  :   } else if (predicate_type_ == PredicateType::InList) {
  : if (values_.size() != other.values_.size()) return false;
  : for (int i = 0; i < values_.size(); i++) {
  :   if (column_.type_info()->Compare(values_[i], 
other.values_[i]) != 0) return false;
  : }
  :   }
May be, it's time to start using the switch() here instead?


PS12, Line 537: other
Is this by-copy capture?  Consider replacing with by-reference capture to avoid 
copying.


PS12, Line 532: void ColumnPredicate::MergeInLists(const ColumnPredicate& 
other) {
  :   // We iterate through the values_ list and remove elements 
that dont
  :   // exist in the other ColumnPredicate's list
  :   // Each list vector is sorted and contains only unique values.
  :   values_.erase(std::remove_if(values_.begin(), values_.end(),
  :[this, other](const void *v) {
  :  return 
!other.CheckValueInList(v);
  :  
This looks strange to me: given the name of the method, I would expect it to 
_merge_ lists, i.e. add the values from other.values_ which are not present in 
this->values_

Consider renaming this method to something like 'IntersectInLists'.


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

PS12, Line 154: if (predicate->predicate_type() == PredicateType::Equality) 
{
  :   memcpy(row->mutable_cell_ptr(*col_idx_it), 
predicate->raw_lower(), size);
  :   pushed_predicates++;
  :   final_predicate = predicate;
  : } else if (predicate->predicate_type() == 
PredicateType::Range) {
  :   if (predicate->raw_upper() != nullptr) {
  : memcpy(row->mutable_cell_ptr(*col_idx_it), 
predicate->raw_upper(), size);
  : pushed_predicates++;
  : final_predicate = predicate;
  :   }
  :   // After the first column with a range constraint we stop 
pushing
  :   // constraints into the upper bound. Instead, we push 
minimum values
  :   // to the remaining columns (below), which is the 
maximally tight
  :   // constraint.
  :   break;
  : } else if (predicate->predicate_type() == 
PredicateType::InList) {
  :   // Since the InList predicate is a sorted vector of 
values, the last
  :   // value would provide an Inclusive Upper Bound that can 
be pushed.
  :   memcpy(row->mutable_cell_ptr(*col_idx_it), 
predicate->raw_values().back(), size);
  :   pushed_predicates++;
  :   final_predicate = predicate;
  : } else {
  :

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-23 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change.

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


Patch Set 14: Code-Review+2

-- 
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: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-23 Thread Dan Burkert (Code Review) 
Dan Burkert has posted comments on this change.

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


Patch Set 9:

(1 comment)

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

Line 122:   vector vals_list;
> Not done?
Woops, had a bad rebase.  It was done in revision 11, and then I somehow lost 
it in 12.


-- 
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: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-23 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change.

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


Patch Set 13:

(1 comment)

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

Line 122:   vector vals_list;
> Done
Not done?


-- 
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: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-23 Thread Alexey Serbin (Code Review) 
Alexey Serbin has posted comments on this change.

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


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/2986/12/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:

Line 106:   int CountMatchedRows(vector values, vector test_values) {
const vector& values?
const vector& test_values?


-- 
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: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-23 Thread Dan Burkert (Code Review) 
Dan Burkert has posted comments on this change.

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


Patch Set 12: Verified+1

-- 
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: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-23 Thread Dan Burkert (Code Review) 
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/2986

to look at the new patch set (#12).

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

KUDU-1363: Add IN-list predicate type

Adds support for clients to provide a set of equalities for a given
column. Support for using IN list predicates from the Java client will
be in a follow-up commit.

Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/scan_predicate.h
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol-test.cc
M src/kudu/common/wire_protocol.cc
16 files changed, 723 insertions(+), 17 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/2986/12
-- 
To view, visit http://gerrit.cloudera.org:8080/2986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-23 Thread Dan Burkert (Code Review) 
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/2986

to look at the new patch set (#11).

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

KUDU-1363: Add IN-list predicate type

Adds support for clients to provide a set of equalities for a given
column. Support for using IN list predicates from the Java client will
be in a follow-up commit.

Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/scan_predicate.h
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol-test.cc
M src/kudu/common/wire_protocol.cc
16 files changed, 724 insertions(+), 17 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/2986/11
-- 
To view, visit http://gerrit.cloudera.org:8080/2986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-23 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change.

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


Patch Set 9:

(1 comment)

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

Line 120: Status InListPredicateData::AddToScanSpec(ScanSpec* spec, Arena* 
arena) {
> Not much I can do about this, it's a virtual method.
You can comment out the parameter name, like 'Arena* /*arena*/' and it'll 
squelch the warning.


-- 
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: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-23 Thread Dan Burkert (Code Review) 
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/2986

to look at the new patch set (#10).

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

KUDU-1363: Add IN-list predicate type

Adds support for clients to provide a set of equalities for a given
column. Support for using IN list predicates from the Java client will
be in a follow-up commit.

Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/scan_predicate.h
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol-test.cc
M src/kudu/common/wire_protocol.cc
16 files changed, 720 insertions(+), 17 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/2986/10
-- 
To view, visit http://gerrit.cloudera.org:8080/2986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1363: Add IN-list predicate type

2016-09-23 Thread Dan Burkert (Code Review) 
Dan Burkert has posted comments on this change.

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


Patch Set 9:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/2986/9/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:

Line 38: using std::set;
> warning: using decl 'set' is unused [misc-unused-using-decls]
Done


http://gerrit.cloudera.org:8080/#/c/2986/7/src/kudu/client/scan_predicate-internal.h
File src/kudu/client/scan_predicate-internal.h:

Line 112:   std::vector vals_;
> This wasn't possible?
It's possible, but it would require copying the vector into a 
vector in the ctor.  I think it's easier to just keep it 
the way it is.


http://gerrit.cloudera.org:8080/#/c/2986/9/src/kudu/client/scan_predicate-internal.h
File src/kudu/client/scan_predicate-internal.h:

Line 23: #include "kudu/client/value.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done


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

Line 120: Status InListPredicateData::AddToScanSpec(ScanSpec* spec, Arena* 
arena) {
> warning: parameter 'arena' is unused [misc-unused-parameters]
Not much I can do about this, it's a virtual method.


Line 122:   vector vals_list;
> Size this up front with vals_.size()?
Done


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

Line 181:   if (values_.size() == 0) {
> warning: the 'empty' method should be used to check for emptiness instead o
Done


-- 
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: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes