David Ribeiro Alves has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9496 )

Change subject: KUDU-721: [Python] Add DECIMAL column type support
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/9496/2/python/kudu/client.pyx
File python/kudu/client.pyx:

http://gerrit.cloudera.org:8080/#/c/9496/2/python/kudu/client.pyx@1324
PS2, Line 1324:     cdef inline get_scale(self, int i):
> The default is 0 for scale because that's the common default for most datab
k, thanks for the explanation


http://gerrit.cloudera.org:8080/#/c/9496/2/python/kudu/libkudu_client.pxd
File python/kudu/libkudu_client.pxd:

http://gerrit.cloudera.org:8080/#/c/9496/2/python/kudu/libkudu_client.pxd@65
PS2, Line 65: cdef extern from "kudu/util/int128.h" namespace "kudu" nogil:
            :     ctypedef int int128_t
> int128_t isn't actually a c++ type. We define it in int128.h based on signe
that's fair regarding the typedef, regarding the nogil: the gil or "global 
interpreter lock" is python's master lock that is held at all times python code 
is being executed (cpython is single threaded). When you step out of python 
though, like when you're calling C++/C code, you can choose to drop it (and get 
some parallelism that way, that's what numpy and other libraries do).

Dropping the gil only makes sense if you're actually executing something 
though, here it seems like you're just using a typedef so not executing 
anything, dropping the lock should have no effect.


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

http://gerrit.cloudera.org:8080/#/c/9496/3/python/kudu/tests/test_scanner.py@263
PS3, Line 263:     def test_decimal_pred(self):
             :         # Test a decimal predicate
             :         # Does a row check count only
why is decimal row check count only?


http://gerrit.cloudera.org:8080/#/c/9496/3/python/kudu/util.py
File python/kudu/util.py:

http://gerrit.cloudera.org:8080/#/c/9496/3/python/kudu/util.py@105
PS3, Line 105:
I think the style is to have two lines between methods. can you add one here 
too (since you added one above?)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e0855100ab1ea891f990931ec94d0b98c0dece1
Gerrit-Change-Number: 9496
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <granthe...@gmail.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com>
Gerrit-Reviewer: Grant Henke <granthe...@gmail.com>
Gerrit-Reviewer: Jordan Birdsell <jtbirds...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Wes McKinney <w...@apache.org>
Gerrit-Reviewer: a...@phdata.io
Gerrit-Comment-Date: Tue, 06 Mar 2018 18:50:18 +0000
Gerrit-HasComments: Yes

Reply via email to