Grant Henke 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: (6 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@1319 PS2, Line 1319: cdef inline get_unscaled_decimal(self, int i): > should this also have a method to get the scaled decimal? seems weird that Actually this should probably just be private and I should just have a public get_decimal. I was using this for get_slot and not really thinking about the public usage. It looks like get_unixtime_micros does something similar where get_slot does the transformation but there isn't a getter to do so. That might be a good change for a different patch too. http://gerrit.cloudera.org:8080/#/c/9496/2/python/kudu/client.pyx@1319 PS2, Line 1319: cdef inline get_unscaled_decimal(self, int i): : cdef int128_t val : check_status(self.row.GetUnscaledDecimal(i, &val)) : return val > worth a note on what is an unscaled decimal With this being private (comment above) we shouldn't need to explain it. http://gerrit.cloudera.org:8080/#/c/9496/2/python/kudu/client.pyx@1324 PS2, Line 1324: cdef inline get_scale(self, int i): > it's not clear that this "scale" refers to scale if this is a decimal, hard The default is 0 for scale because that's the common default for most databases. We default to 0 for precision for convenience on non-decimal columns and also to represent an "invalid" precision in cases where type attributes is used on a non-decimal column. http://gerrit.cloudera.org:8080/#/c/9496/2/python/kudu/client.pyx@2468 PS2, Line 2468: to_unscaled_decimal(value))) > nit: indentation Done 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 > do we really need this extern to get a typedef and, if we do, do we need th int128_t isn't actually a c++ type. We define it in int128.h based on signed __int128. Without this all the int128_t usage below fails. Reading up on GIL and nogil it looks like it's something that should be done for thread safe code. Which I suspect a typedef has no issues with. It's used quite a bit in this file. http://cython.readthedocs.io/en/latest/src/userguide/external_C_code.html#acquiring-and-releasing-the-gil http://gerrit.cloudera.org:8080/#/c/9496/2/python/kudu/tests/test_scantoken.py File python/kudu/tests/test_scantoken.py: http://gerrit.cloudera.org:8080/#/c/9496/2/python/kudu/tests/test_scantoken.py@246 PS2, Line 246: # Test unixtime_micros value predicate > copy pasta artifact Done -- 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 02:03:04 +0000 Gerrit-HasComments: Yes
