Will Berkeley has posted comments on this change. Change subject: KUDU-1614 - [python] Enable Set/Get of unixtime_micros ......................................................................
Patch Set 3: (5 comments) Looks nice; mostly nits. It's good to be able to use a datetime to set a timestamp-- I think this makes Python client support for the unixtime_micros column exceed that of the Java and C++ clients! http://gerrit.cloudera.org:8080/#/c/4417/3/python/kudu/client.pyx File python/kudu/client.pyx: PS3, Line 1743: # Leave it Nit: extra whitespace here and the next two lines. http://gerrit.cloudera.org:8080/#/c/4417/3/python/kudu/libkudu_client.pxd File python/kudu/libkudu_client.pxd: PS3, Line 217: Status GetUnixTimeMicros(int col_idx, Nit: could you do me a favor and reorganize these so either all the name-based getters are together and all the index-based getters are together (same order of types within each group), or each type-specific pair of getters (one name-based, one index-based) is together? Thanks! http://gerrit.cloudera.org:8080/#/c/4417/3/python/kudu/tests/common.py File python/kudu/tests/common.py: PS3, Line 167: 'timestamp_val' Nit: rename to unixtime_micros_val or something. There may eventually be a timestamp type again, one that is more aligned with a timestamp type used by Impala or elsewhere in the Hadoopiverse, so may as well avoid the potential collision now. http://gerrit.cloudera.org:8080/#/c/4417/3/python/kudu/tests/test_scantoken.py File python/kudu/tests/test_scantoken.py: PS3, Line 173: # Insert new rows Is there a test_util.py or something where this duplicated block of code could be shared by the scanner and scan token tests? http://gerrit.cloudera.org:8080/#/c/4417/3/python/kudu/util.py File python/kudu/util.py: PS3, Line 75: if isinstance(unixtime_micros, int): Missing else? -- To view, visit http://gerrit.cloudera.org:8080/4417 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id428cbd072b7de7a75e58b66e4de89acd381fdca Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jordan Birdsell <jordantbirds...@gmail.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Jordan Birdsell <jordantbirds...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-HasComments: Yes
