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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Jordan Birdsell <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Wes McKinney <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Comment-Date: Tue, 06 Mar 2018 18:50:18 +0000 Gerrit-HasComments: Yes
