Wes McKinney has posted comments on this change. ( http://gerrit.cloudera.org:8080/15661 )
Change subject: python: support for passing arrow data to python ...................................................................... Patch Set 4: (10 comments) Thanks for updating this -- I left a few comments mostly about the pyarrow/Cython portions http://gerrit.cloudera.org:8080/#/c/15661/4/python/kudu/client.pyx File python/kudu/client.pyx: http://gerrit.cloudera.org:8080/#/c/15661/4/python/kudu/client.pyx@60 PS4, Line 60: CLIENT_SUPPORTS_ARROW = False A version check might not be a bad idea since the C interface didn't appear in pyarrow until ~0.17.0 http://gerrit.cloudera.org:8080/#/c/15661/4/python/kudu/client.pyx@2089 PS4, Line 2089: check_status(self.scanner.NextBatch(&batch)) Recommend `with nogil:` block here http://gerrit.cloudera.org:8080/#/c/15661/4/python/kudu/client.pyx@2091 PS4, Line 2091: return _empty_batch() In principle this 0-row case shouldn't be necessary? http://gerrit.cloudera.org:8080/#/c/15661/4/python/kudu/client.pyx@2092 PS4, Line 2092: check_status(batch.ToArrow(&arrow)) with nogil: http://gerrit.cloudera.org:8080/#/c/15661/4/python/kudu/client.pyx@2224 PS4, Line 2224: if self.columnar_enabled: Can we have a `to_arrow()` method, too? Then `to_pandas()` is just `self.to_arrow().to_pandas()` http://gerrit.cloudera.org:8080/#/c/15661/4/python/kudu/client.pyx@2230 PS4, Line 2230: if arrow_batch and arrow_batch.num_rows != 0 ] This formatting is a bit odd, generally we might see something like [arrow_batch for arrow_batch in self.xbatches_arrow() if arrow_batch and arrow_batch.num_rows != 0] http://gerrit.cloudera.org:8080/#/c/15661/4/python/kudu/schema.pyx File python/kudu/schema.pyx: http://gerrit.cloudera.org:8080/#/c/15661/4/python/kudu/schema.pyx@785 PS4, Line 785: check_status(self.schema.ToArrowSchema(&arrow_schema)) with nogil: http://gerrit.cloudera.org:8080/#/c/15661/4/python/kudu/tests/test_scanner.py File python/kudu/tests/test_scanner.py: http://gerrit.cloudera.org:8080/#/c/15661/4/python/kudu/tests/test_scanner.py@407 PS4, Line 407: Should add TODO to improve these tests to test functionality without requiring pandas (which may in turn require implementing `to_arrow()` when columnar layout is disabled, so more work required) http://gerrit.cloudera.org:8080/#/c/15661/4/python/kudu/tests/util.py File python/kudu/tests/util.py: http://gerrit.cloudera.org:8080/#/c/15661/4/python/kudu/tests/util.py@71 PS4, Line 71: # use precision>18 to ensure we use DECIMAL128. NB: someone is working on this right now but it might be 3-6 months before it could get picked up by Kudu. Is there a way to promote decimal32/64 to decimal128 in the meantime? http://gerrit.cloudera.org:8080/#/c/15661/4/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/15661/4/src/kudu/client/client-test.cc@1131 PS4, Line 1131: scanner.GetProjectionSchema().ToArrowSchema(&as); unchecked status? -- To view, visit http://gerrit.cloudera.org:8080/15661 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5df4714bf5ac339d675f2a9169f8aeff06d30eed Gerrit-Change-Number: 15661 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wes McKinney <[email protected]> Gerrit-Comment-Date: Sun, 18 Oct 2020 23:04:31 +0000 Gerrit-HasComments: Yes
