[email protected] 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: [email protected] Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: [email protected] Gerrit-Comment-Date: Mon, 16 Apr 2018 19:48:19 +0000 Gerrit-HasComments: Yes
