You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Jordan Birdsell (Code Review)" <ge...@cloudera.org> on 2016/09/28 01:16:03 UTC

[kudu-CR] [WIP]KUDU-1640 - [python] Add IN-list predicate support

Jordan Birdsell has uploaded a new change for review.

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

Change subject: [WIP]KUDU-1640 - [python] Add IN-list predicate support
......................................................................

[WIP]KUDU-1640 - [python] Add IN-list predicate support

This patch adds IN list predicate support for the python client.
This patch includes a test.

Change-Id: I932dfded62e162cf85e0e12432cf6716311957de
---
M python/kudu/client.pyx
M python/kudu/libkudu_client.pxd
M python/kudu/tests/test_scanner.py
3 files changed, 27 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/4548/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4548
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I932dfded62e162cf85e0e12432cf6716311957de
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell <jo...@gmail.com>

[kudu-CR] KUDU-1640 - [python] Add IN-list predicate support

Posted by "Jordan Birdsell (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1640 - [python] Add IN-list predicate support
......................................................................

KUDU-1640 - [python] Add IN-list predicate support

This patch adds IN list predicate support for the python client.
This patch includes a test.

Change-Id: I932dfded62e162cf85e0e12432cf6716311957de
---
M python/kudu/client.pyx
M python/kudu/libkudu_client.pxd
M python/kudu/tests/test_scanner.py
M python/kudu/tests/test_scantoken.py
4 files changed, 125 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/4548/6
-- 
To view, visit http://gerrit.cloudera.org:8080/4548
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I932dfded62e162cf85e0e12432cf6716311957de
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell <jo...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jordan Birdsell <jo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1640 - [python] Add IN-list predicate support

Posted by "Jordan Birdsell (Code Review)" <ge...@cloudera.org>.

Jordan Birdsell has posted comments on this change.

Change subject: KUDU-1640 - [python] Add IN-list predicate support
......................................................................


Patch Set 7:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4548/6/python/kudu/client.pyx
File python/kudu/client.pyx:

Line 816:         col_name_slice = Slice(<char*> _name, len(_name))
> This is already done by the C++ client, so you shouldn't need to do it here
Done


Line 819:             for val in values:
> is this returning a boolean?
no, the column class overrides the == comparison operator and instead creates a predicate. See the __richcmp__ method.  At any rate, removed since this is addressed with the C++ client.


Line 827: 
> The issue isn't clearing the vector, but instead freeing all of the contain
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I932dfded62e162cf85e0e12432cf6716311957de
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell <jo...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jordan Birdsell <jo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1640 - [python] Add IN-list predicate support

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.

Dan Burkert has posted comments on this change.

Change subject: KUDU-1640 - [python] Add IN-list predicate support
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4548/3/python/kudu/client.pyx
File python/kudu/client.pyx:

Line 812:         del col_name_slice
> Doesnt the try finally take care of this? Arent we essentially moving it by
no, in this case dereferencing it would be a copy.  Copy vs. move isn't really the important part; the bigger issue is that col_name_slice could be stack allocated, in which case you don't have to worry about deleting it at all.  Copying and moving a slice are pretty much the same thing, since they don't own the data they point to.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I932dfded62e162cf85e0e12432cf6716311957de
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell <jo...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jordan Birdsell <jo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1640 - [python] Add IN-list predicate support

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.

Dan Burkert has posted comments on this change.

Change subject: KUDU-1640 - [python] Add IN-list predicate support
......................................................................


Patch Set 5:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4548/5/python/kudu/client.pyx
File python/kudu/client.pyx:

Line 768:                                    len(self.name))
fixup indentation


Line 822:                                      len(self.name))
fixup indentation


Line 824:                 vals.push_back(self.box_value(val))
If box_value throws a TypeError, will this leak the already constructed values in vals?


http://gerrit.cloudera.org:8080/#/c/4548/5/python/kudu/tests/test_scanner.py
File python/kudu/tests/test_scanner.py:

Line 77:     def test_scan_rows_in_list_predicate(self):
Could you add a test with mistyped values in the list, both that the python KuduValue converter can't handle (e.g. a python class for a string column), and also mistyped in terms of the wrong type for the column (e.g. int for a string column).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I932dfded62e162cf85e0e12432cf6716311957de
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell <jo...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jordan Birdsell <jo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1640 - [python] Add IN-list predicate support

Posted by "Jordan Birdsell (Code Review)" <ge...@cloudera.org>.

Jordan Birdsell has posted comments on this change.

Change subject: KUDU-1640 - [python] Add IN-list predicate support
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4548/3/python/kudu/client.pyx
File python/kudu/client.pyx:

Line 812:             vector[KuduValue*] vals
> no, in this case dereferencing it would be a copy.  Copy vs. move isn't rea
sorry for the confusion, done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I932dfded62e162cf85e0e12432cf6716311957de
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell <jo...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jordan Birdsell <jo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes