a...@phdata.io has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10072 )

Change subject: KUDU-2399: Support IS NULL / IS NOT NULL predicates in Python
......................................................................


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/10072/4/python/kudu/client.pyx
File python/kudu/client.pyx:

http://gerrit.cloudera.org:8080/#/c/10072/4/python/kudu/client.pyx@989
PS4, Line 989:         Creates a new IsNotNullPredicate for the Column which 
can be used for scanners on this tabl
> nit: lines should be <= 100 cols
I'll fix this.


http://gerrit.cloudera.org:8080/#/c/10072/4/python/kudu/tests/common.py
File python/kudu/tests/common.py:

http://gerrit.cloudera.org:8080/#/c/10072/4/python/kudu/tests/common.py@117
PS4, Line 117: default=None
> this is quite different, it doesn't break tests?
It didn't break any of the existing tests, seemed like the easiest way to 
implement null rows to test against. Looked like the original intention was to 
create every other row as string_val null. None of the existing tests 
specifically checked a 'nothing' value in a string_val column.


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

http://gerrit.cloudera.org:8080/#/c/10072/4/python/kudu/tests/test_scanner.py@75
PS4, Line 75:         scanner.set_projected_column_indexes([0, 2])
> why this change?
This test was already broken as the title says it was supposed to test a 
predicate an projection and there was no projection. I didn't want it to be 
attributed to my changes to None in test/common.py, but It can certainly be 
moved to another patch.


http://gerrit.cloudera.org:8080/#/c/10072/4/python/kudu/tests/util.py
File python/kudu/tests/util.py:

http://gerrit.cloudera.org:8080/#/c/10072/4/python/kudu/tests/util.py@a62
PS4, Line 62:
            :
            :
            :
            :
> aren't we testing less with this change?
With "'hello_%d' % i if i % 2 == 0 else None," on line 48 we are creating None 
column values but then replacing them with string 'nothing' here this is 
because when they got inserted they would be None based on common.py 
default='nothing'. As far as I could tell none of the tests were currently 
using this.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I52a22d6d8a8fe9a6049270596eb131799c37f82f
Gerrit-Change-Number: 10072
Gerrit-PatchSet: 4
Gerrit-Owner: a...@phdata.io
Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com>
Gerrit-Reviewer: Grant Henke <granthe...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: a...@phdata.io
Gerrit-Comment-Date: Mon, 16 Apr 2018 19:48:19 +0000
Gerrit-HasComments: Yes

Reply via email to