Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Python] Incorrect return type for FlightStreamReader.read_chunk() #34017

Closed
ravjotbrar opened this issue Feb 3, 2023 · 4 comments · Fixed by #38641
Closed

[Python] Incorrect return type for FlightStreamReader.read_chunk() #34017

ravjotbrar opened this issue Feb 3, 2023 · 4 comments · Fixed by #38641
Assignees
@nph
Labels
Component: Documentation Component: Python good-first-issue Type: bug
Milestone
13.0.0

Comments

@ravjotbrar
Copy link
Contributor

ravjotbrar commented Feb 3, 2023 •
edited
Loading

Describe the bug, including details regarding any error messages, version, and platform.

The documentation for using the FlightStreamReader to read_chunk currently states that this method returns a RecordBatch. The return type is actually FlightStreamChunk, which then contains an attribute called data that is of type RecordBatch.

When trying to click "Edit this Page", I get a 404 error: https://github.com/apache/arrow/edit/master/docs/source/python/generated/pyarrow.flight.FlightStreamReader.rst

Component(s)

Documentation, Python
@AkashS20
Copy link

AkashS20 commented Feb 4, 2023

Hi, I would like to contribute to this. I'm new to open source so if you could please guide me on what is to be done and what are the pre-requisites for the same, it will be of great help.

jjerphan added a commit to jjerphan/arrow that referenced this issue May 13, 2023
@jjerphan
apacheGH-34017: [Python] Fix FlightStreamReader.read_chunk's docstring
9126335 
Signed-off-by: Julien Jerphanion <[email protected]>
@jjerphan jjerphan mentioned this issue May 13, 2023
Merged
lidavidm pushed a commit that referenced this issue May 15, 2023
@jjerphan
GH-34017: [Python][FlightRPC][Doc] Fix FlightStreamReader.read_chunk'…
18c012a 
…s docstring (#35583)

### Rationale for this change

Addresses #34017.

### What changes are included in this PR?

Simple docstring fixes.

### Are these changes tested?

No (must they be test?)

### Are there any user-facing changes?

Yes.

Authored-by: Julien Jerphanion <[email protected]>
Signed-off-by: David Li <[email protected]>
@lidavidm lidavidm added this to the 13.0.0 milestone May 15, 2023
@lidavidm
Copy link
Member

Issue resolved by pull request 35583
#35583

ArgusLi pushed a commit to Bit-Quill/arrow that referenced this issue May 15, 2023
@jjerphan @ArgusLi
apacheGH-34017: [Python][FlightRPC][Doc] Fix FlightStreamReader.read_…
e8433bc 
…chunk's docstring (apache#35583)

### Rationale for this change

Addresses apache#34017.

### What changes are included in this PR?

Simple docstring fixes.

### Are these changes tested?

No (must they be test?)

### Are there any user-facing changes?

Yes.

Authored-by: Julien Jerphanion <[email protected]>
Signed-off-by: David Li <[email protected]>
rtpsw pushed a commit to rtpsw/arrow that referenced this issue May 16, 2023
@jjerphan @rtpsw
apacheGH-34017: [Python][FlightRPC][Doc] Fix FlightStreamReader.read_…
c80727a 
…chunk's docstring (apache#35583)

### Rationale for this change

Addresses apache#34017.

### What changes are included in this PR?

Simple docstring fixes.

### Are these changes tested?

No (must they be test?)

### Are there any user-facing changes?

Yes.

Authored-by: Julien Jerphanion <[email protected]>
Signed-off-by: David Li <[email protected]>
@nph
Copy link
Contributor

nph commented Nov 8, 2023 •
edited
Loading

@lidavidm @jjerphan FYI the docs for FlightStreamReader.read_chunk are still wrong. They currently show that read_chunk returns tuple[FlightStreamChunk, Buffer | None], however it returns only a FlightStreamChunk instance. The FlightStreamChunk class supports tuple unpacking into a tuple[RecordBatch, Buffer | None], which is presumably why the original docstring showed this as the return type.

Shall I go ahead an open a PR for this?

@jjerphan
Copy link
Contributor

jjerphan commented Nov 8, 2023

@nph: feel free to open a PR, yes!

@nph nph mentioned this issue Nov 9, 2023
Merged
lidavidm pushed a commit that referenced this issue Nov 9, 2023
@nph
GH-34017: [Python][FlightRPC][Doc] Fix read_chunk docstring for Fli…
80503a6 
…ghtStreamReader and MetadataRecordBatchReader (#38641)

### Rationale for this change

The docs for `FlightStreamReader` and `MetadataRecordBatchReader` currently list an incorrect return type for the `read_chunk` method - see #34017. NB: this issue was partially addressed by #35583.

### What changes are included in this PR?

Simple docstring update for `read_chunk`.

### Are these changes tested?

No (v minor docstring only change)

### Are there any user-facing changes?

Yes
* Closes: #34017

Authored-by: Nick Hughes <[email protected]>
Signed-off-by: David Li <[email protected]>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
@nph @loicalleyne
apacheGH-34017: [Python][FlightRPC][Doc] Fix read_chunk docstring f…
1fd2ad4 
…or FlightStreamReader and MetadataRecordBatchReader (apache#38641)

### Rationale for this change

The docs for `FlightStreamReader` and `MetadataRecordBatchReader` currently list an incorrect return type for the `read_chunk` method - see apache#34017. NB: this issue was partially addressed by apache#35583.

### What changes are included in this PR?

Simple docstring update for `read_chunk`.

### Are these changes tested?

No (v minor docstring only change)

### Are there any user-facing changes?

Yes
* Closes: apache#34017

Authored-by: Nick Hughes <[email protected]>
Signed-off-by: David Li <[email protected]>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
@nph @dgreiss
apacheGH-34017: [Python][FlightRPC][Doc] Fix read_chunk docstring f…
162826a 
…or FlightStreamReader and MetadataRecordBatchReader (apache#38641)

### Rationale for this change

The docs for `FlightStreamReader` and `MetadataRecordBatchReader` currently list an incorrect return type for the `read_chunk` method - see apache#34017. NB: this issue was partially addressed by apache#35583.

### What changes are included in this PR?

Simple docstring update for `read_chunk`.

### Are these changes tested?

No (v minor docstring only change)

### Are there any user-facing changes?

Yes
* Closes: apache#34017

Authored-by: Nick Hughes <[email protected]>
Signed-off-by: David Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nph nph

Labels
Component: Documentation Component: Python good-first-issue Type: bug
Projects
None yet
Milestone
13.0.0
5 participants
@lidavidm @nph @jjerphan @ravjotbrar @AkashS20