Jordan Birdsell has posted comments on this change.

Change subject: KUDU-1671 - [python] Enable predicate pushdown for additional 
types
......................................................................


Patch Set 3:

(5 comments)

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

Line 1797:                 to_unixtime_micros(value))
> just so that I understand you changed to_unixtime_micros to be able to take
yep


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

Line 159:         # Using the incoming list of predicates, verify that the row 
returned
> xxxs nit: wrap this comment so that we use less lines.
Done


PS2, Line 161: 
> typo
Done


Line 189:         self._test_float_pred()
> why?
Python floats are actually doubles (retrieving the value back from kudu casts 
to a double), so verifying the values here will get annoying. I was thinking 
that by confirming row count we at least confirm the predicate portion is 
working (since casts from double to float are ok). Should I add some sort of 
rounded comparison of the value or some bit manipulation/comparison?  Note: In 
the future, likely when pandas is integrated, we will probably use numpy which 
will support the single precision floats.


http://gerrit.cloudera.org:8080/#/c/4589/2/python/kudu/util.py
File python/kudu/util.py:

Line 66:     elif isinstance(timestamp, tuple):
> oh you did.
:)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If5766d24055dfba5fa371fc61c6dfd66adc54273
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell <[email protected]>
Gerrit-Reviewer: David Ribeiro Alves <[email protected]>
Gerrit-Reviewer: Jordan Birdsell <[email protected]>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <[email protected]>
Gerrit-Reviewer: Will Berkeley <[email protected]>
Gerrit-HasComments: Yes

Reply via email to