ARROW-16430: [Python] Add support for reading record batch custom metadata API by niyue · Pull Request #13041 · apache/arrow

niyue · 2022-05-01T06:31:05Z

In ARROW-16131, C++ APIs were added so that users can read/write record batch custom metadata for IPC file. In this PR, pyarrow APIs are added so that python users can take advantage of these APIs to address ARROW-16430.

github-actions · 2022-05-01T06:31:24Z

https://issues.apache.org/jira/browse/ARROW-16430

github-actions · 2022-05-01T06:31:25Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

pitrou · 2022-05-10T10:39:36Z

@jorisvandenbossche Is this the right Numpydoc syntax when a namedtuple is returned?

pitrou · 2022-05-10T10:45:51Z

By the way, this PR addresses RecordBatchFileReader but not RecordBatchReader. Do you plan to do that as well?

niyue · 2022-05-12T07:07:55Z

By the way, this PR addresses RecordBatchFileReader but not RecordBatchReader. Do you plan to do that as well?

I tried adding the pyarrow API for RecordBatchReader locally previously, but I found if I want to test the new pyarrow API for RecordBatchReader, I probably have to add the corresponding API implementation in C++ RecordBatchStreamReader so that I can unit test it in a meaningful way since it seems only RecordBatchStreamReader and RecordBatchFileReader provides the capability to read record batch custom metadata (is this correct?). And since this PR is more about adding new pyarrow API for existing C++ capability, I would like to make non necessary C++ API change less in this PR.

if there is other way to unit test pyarrow RecordBatchReader.read_next_batch_with_metadata API besides implementing it in RecordBatchStreamReader?
do you recommend to put all these changes together in single PR or keep them as separated PRs? I initially submitted this issue as a pyarrow enhancement PR, but I am okay to include more API changes if it is preferred.

niyue · 2022-05-17T02:45:16Z

@pitrou I made some additional changes to the PR, could you please help to review? Thanks.

pitrou · 2022-05-18T15:55:10Z

@niyue Sorry for the late replay, but the C++ RecordBatchReader already has a method Result<RecordBatchWithMetadata> ReadNext(), so you wouldn't need to add anything more to the C++ side AFAICT?

pitrou · 2022-08-09T09:25:05Z

@niyue Are you planning to work on this?
Otherwise @milesgranger you might be interested in taking this up?

milesgranger · 2022-08-09T09:43:04Z

I can certainly pick it up if needed!
Presently, I'm working locally on ARROW-13763.

niyue · 2022-08-09T11:36:18Z

@niyue Are you planning to work on this?

Otherwise @milesgranger you might be interested in taking this up?

Hi @pitrou, sorry I got too busy since last commit and forgot this issue previously. I am still interested working on this, and can start working on it from next week.

pitrou · 2022-08-17T10:03:39Z

@niyue If/when this is ready for review, please say so :-)

niyue · 2022-08-17T10:13:07Z

@pitrou I pushed a new commit a minute ago. I followed your suggestion to add corresponding API for RecordBatchReader API in pyarrow, and a basic unit test to cover this API. I ran all pyarrow tests (using the minimum build only) and all tests passed locally. Could you please help to review? Thanks.

niyue · 2022-09-08T22:52:08Z

@pitrou I am not sure if you see my previous comment. I made some change previously, and this PR is ready for review, could you please help? Thanks.

pitrou · 2022-09-12T14:58:01Z

@niyue Sorry, I had overlooked it. I'll take a look when I can. @jorisvandenbossche would you like to review this too?

pitrou

Sorry for the delay! Here are some more comments.

niyue · 2022-09-22T23:53:11Z

No problem. I will check them out and see if I can get them addressed. Thanks for the review.

pitrou · 2022-10-05T09:02:29Z

There were some timeouts in some CI jobs. I've restarted them just in case. If that still reproduces, should first rebase from master to catch up with any upstream fixes.

jorisvandenbossche

Looks good, just two minor doc formatting comments

jorisvandenbossche · 2022-10-05T09:14:31Z

This looks like a nice addition. Something I am wondering more in general (potentially for future JIRA/PRs), when working with custom metadata, would it be useful to also allow to inspect the custom metadata of a certain batch, without also loading the batch? (so you could for example check the metadata before deciding whether to read the batch or not).
I suppose that would only make sense for the IPC file format? (although not sure if that makes sense at the format / C++ level) Like the RecordBatchFileReader now has a get_batch(i)/ get_batch_with_custom_metadata(i), one could also have a get_custom_metadata(i)?

pitrou · 2022-10-05T09:24:11Z

Something I am wondering more in general (potentially for future JIRA/PRs), when working with custom metadata, would it be useful to also allow to inspect the custom metadata of a certain batch, without also loading the batch?

Perhaps, but that would have to be done on the C++ side first. Also, I would wait for users to actually request it.

…row.

niyue · 2022-10-10T06:16:53Z

@pitrou sorry for the late response. There are two failed CI builds, and they don't seem relevant in this issue. I rebased onto the latest master branch anyway.

…w so that pyarrow can read record batch along with its custom metadata.

niyue · 2022-11-02T08:53:49Z

@pitrou could you please help to review the PR to see if there is still anything we desire to change? Thanks.

pitrou

Really sorry for the delay @niyue ! I've pushed a couple minor changes and will merge if CI is green.

Thank you for the work!

niyue · 2022-12-13T04:50:59Z

No problem @pitrou . Thanks for the review and the fixes.

github-actions Bot added Component: C++ Component: Python labels May 1, 2022

niyue commented May 1, 2022

View reviewed changes

Comment thread cpp/src/arrow/ipc/writer.h Outdated

niyue commented May 1, 2022

View reviewed changes

Comment thread python/pyarrow/includes/libarrow.pxd Outdated

niyue force-pushed the feature/py-batch-custom-meta branch from 1a7e908 to 620ff15 Compare May 1, 2022 07:06

github-actions Bot added the Component: FlightRPC label May 1, 2022

niyue force-pushed the feature/py-batch-custom-meta branch 3 times, most recently from 34b556a to 38e45dd Compare May 1, 2022 08:59

kou changed the title ~~ARROW-16430, support reading record batch custom metadata API in pyarrow~~ ARROW-16430: [Python] Add support for reading record batch custom metadata API May 6, 2022

niyue force-pushed the feature/py-batch-custom-meta branch from 38e45dd to c1fb6ad Compare May 7, 2022 01:35

pitrou reviewed May 10, 2022

View reviewed changes

Comment thread python/pyarrow/_flight.pyx Outdated

pitrou reviewed May 10, 2022

View reviewed changes

Comment thread python/pyarrow/ipc.pxi Outdated

pitrou reviewed May 10, 2022

View reviewed changes

Comment thread python/pyarrow/tests/test_flight.py Outdated

niyue force-pushed the feature/py-batch-custom-meta branch from c1fb6ad to 78f492f Compare May 12, 2022 05:41

niyue force-pushed the feature/py-batch-custom-meta branch from 78f492f to 3f040a0 Compare May 12, 2022 07:24

niyue force-pushed the feature/py-batch-custom-meta branch from 3f040a0 to a676680 Compare August 17, 2022 06:24

pitrou reviewed Sep 22, 2022

View reviewed changes

niyue force-pushed the feature/py-batch-custom-meta branch 2 times, most recently from 6498958 to ef95d59 Compare September 27, 2022 10:18

jorisvandenbossche reviewed Oct 5, 2022

View reviewed changes

Comment thread python/pyarrow/ipc.pxi Outdated

Comment thread python/pyarrow/tests/test_ipc.py Outdated

jorisvandenbossche approved these changes Oct 5, 2022

View reviewed changes

Comment thread python/pyarrow/ipc.pxi Outdated

Comment thread python/pyarrow/ipc.pxi Outdated

ARROW-16430, support reading record batch custom metadata API in pyar…

376b519

…row.

niyue force-pushed the feature/py-batch-custom-meta branch from ef95d59 to af3cd34 Compare October 10, 2022 06:13

Add ReadNext with custom metadata API for RecordBatchReader in pyarro…

8f5d119

…w so that pyarrow can read record batch along with its custom metadata.

niyue force-pushed the feature/py-batch-custom-meta branch from af3cd34 to 8f5d119 Compare October 10, 2022 06:20

pitrou added 2 commits December 12, 2022 18:08

Merge branch 'master' into feature/py-batch-custom-meta

36b4ad4

Parametrize test, some docstring nits

cf65477

pitrou approved these changes Dec 12, 2022

View reviewed changes

pitrou merged commit 45d83fe into apache:master Dec 12, 2022

asfimport mentioned this pull request Dec 13, 2022

[Python] Read/Write record batch custom metadata API in pyarrow #31805

Closed

Conversation

niyue commented May 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 1, 2022

Uh oh!

github-actions Bot commented May 1, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou May 10, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou commented May 10, 2022

Uh oh!

niyue commented May 12, 2022

Uh oh!

niyue commented May 17, 2022

Uh oh!

pitrou commented May 18, 2022

Uh oh!

pitrou commented Aug 9, 2022

Uh oh!

milesgranger commented Aug 9, 2022

Uh oh!

niyue commented Aug 9, 2022

Uh oh!

pitrou commented Aug 17, 2022

Uh oh!

niyue commented Aug 17, 2022

Uh oh!

niyue commented Sep 8, 2022

Uh oh!

pitrou commented Sep 12, 2022

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niyue commented Sep 22, 2022

Uh oh!

Uh oh!

Uh oh!

pitrou commented Oct 5, 2022

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jorisvandenbossche commented Oct 5, 2022

Uh oh!

pitrou commented Oct 5, 2022

Uh oh!

niyue commented Oct 10, 2022

Uh oh!

niyue commented Nov 2, 2022

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

niyue commented Dec 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

niyue commented May 1, 2022 •

edited

Loading