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: (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 the get_slot method returns a decimal, but the caller has to call from_unscaled_decimal if using the particular getter 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 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 to navigate to the c++ header, since this is python. likely docs enough. on a totally side note, as I was browsing ColumnTypeAttributes i noticed that the defaults for precision and scale are 0. was wondering why not 1. http://gerrit.cloudera.org:8080/#/c/9496/2/python/kudu/client.pyx@2468 PS2, Line 2468: to_unscaled_decimal(value))) nit: indentation 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 the nogil? 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 -- 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 00:45:21 +0000 Gerrit-HasComments: Yes
