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.

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

[C++] RecordBatchWriter::Close does not prevent future writes #35095

Closed
cih9088 opened this issue Apr 13, 2023 · 5 comments · Fixed by #37783
Closed

[C++] RecordBatchWriter::Close does not prevent future writes #35095

cih9088 opened this issue Apr 13, 2023 · 5 comments · Fixed by #37783

Comments

@cih9088
Copy link

cih9088 commented Apr 13, 2023

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

import pyarrow as pa

data = [
    pa.array([1, 2, 3, 4]),
    pa.array(['foo', 'bar', 'baz', None]),
    pa.array([True, None, False, True])
]

batch = pa.record_batch(data, names=['f0', 'f1', 'f2'])

f = pa.ipc.new_stream('test.arrow', batch.schema)
f.write_batch(batch)
f.close()
# still writable
f.write_batch(batch)

with pa.ipc.new_file('test2.arrow', batch.schema) as f2:
    f2.write_batch(batch)
# still writable
f2.write_batch(batch)

Maybe related to #34423?
I tried with gc but no luck.

Component(s)

Python

@westonpace
Copy link
Member

Good catch. I'm going to move this one to C++ since the fix should probably be on the arrow::ipc::RecordBatchWriter C++ class. I think it would be fairly straightforward.

@westonpace westonpace changed the title [Python] pyarrow does not close object [C++] pyarrow does not close object Apr 17, 2023
@westonpace westonpace changed the title [C++] pyarrow does not close object [C++] RecordBatchWriter::Close does not prevent future writes Apr 17, 2023
@Ziy1-Tan
Copy link

take

@jjerphan
Copy link
Contributor

Hi @Ziy1-Tan, are you still planning to work on this PR?

@Ziy1-Tan
Copy link

Hey, @jjerphan! Sorry, I'm busy this period. Free free to take this:)

@Ziy1-Tan Ziy1-Tan removed their assignment May 16, 2023
chrisjordansquire added a commit to chrisjordansquire/arrow that referenced this issue Sep 18, 2023
This addresses apacheGH-35095 by adding a flag to IpcFormatWriter
to track when a writer has been closed, and check this flag
before writes.
@chrisjordansquire
Copy link
Contributor

@westonpace - I created a PR for this issue, #37783 . Review would be greatly appreciated!

chrisjordansquire added a commit to chrisjordansquire/arrow that referenced this issue Sep 20, 2023
This addresses apacheGH-35095 by adding a flag to IpcFormatWriter
to track when a writer has been closed, and check this flag
before writes.
chrisjordansquire added a commit to chrisjordansquire/arrow that referenced this issue Sep 20, 2023
This addresses apacheGH-35095 by adding a flag to IpcFormatWriter
to track when a writer has been closed, and check this flag
before writes.
kou added a commit that referenced this issue Sep 21, 2023
…ter (#37783)

This addresses GH-35095 by adding a flag to IpcFormatWriter to track when a writer has been closed, and check this flag before writes.

### Rationale for this change

This addresses #35095 , preventing stream and file IPC writers from writing record batches once the IPC writer has been closed. 

### What changes are included in this PR?

Adding a flag so that an IpcFormatWriter to track when it's been closed, a check before writes in IpcFormatWriter, and two tests to confirm it works as expected. 

### Are these changes tested?

Yes, the changes are tested. The two tests were added, and the C++ test suite ran. No unexpected failures appeared. 

### Are there any user-facing changes?

Other than newly returning an invalid status when writing after close, no, there should not be any user-facing changes. 

* Closes: #35095

Lead-authored-by: Chris Jordan-Squire <chris.jordansquire@gmail.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@kou kou added this to the 14.0.0 milestone Sep 21, 2023
etseidl pushed a commit to etseidl/arrow that referenced this issue Sep 28, 2023
…matWriter (apache#37783)

This addresses apacheGH-35095 by adding a flag to IpcFormatWriter to track when a writer has been closed, and check this flag before writes.

### Rationale for this change

This addresses apache#35095 , preventing stream and file IPC writers from writing record batches once the IPC writer has been closed. 

### What changes are included in this PR?

Adding a flag so that an IpcFormatWriter to track when it's been closed, a check before writes in IpcFormatWriter, and two tests to confirm it works as expected. 

### Are these changes tested?

Yes, the changes are tested. The two tests were added, and the C++ test suite ran. No unexpected failures appeared. 

### Are there any user-facing changes?

Other than newly returning an invalid status when writing after close, no, there should not be any user-facing changes. 

* Closes: apache#35095

Lead-authored-by: Chris Jordan-Squire <chris.jordansquire@gmail.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
JerAguilon pushed a commit to JerAguilon/arrow that referenced this issue Oct 23, 2023
…matWriter (apache#37783)

This addresses apacheGH-35095 by adding a flag to IpcFormatWriter to track when a writer has been closed, and check this flag before writes.

### Rationale for this change

This addresses apache#35095 , preventing stream and file IPC writers from writing record batches once the IPC writer has been closed. 

### What changes are included in this PR?

Adding a flag so that an IpcFormatWriter to track when it's been closed, a check before writes in IpcFormatWriter, and two tests to confirm it works as expected. 

### Are these changes tested?

Yes, the changes are tested. The two tests were added, and the C++ test suite ran. No unexpected failures appeared. 

### Are there any user-facing changes?

Other than newly returning an invalid status when writing after close, no, there should not be any user-facing changes. 

* Closes: apache#35095

Lead-authored-by: Chris Jordan-Squire <chris.jordansquire@gmail.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
…matWriter (apache#37783)

This addresses apacheGH-35095 by adding a flag to IpcFormatWriter to track when a writer has been closed, and check this flag before writes.

### Rationale for this change

This addresses apache#35095 , preventing stream and file IPC writers from writing record batches once the IPC writer has been closed. 

### What changes are included in this PR?

Adding a flag so that an IpcFormatWriter to track when it's been closed, a check before writes in IpcFormatWriter, and two tests to confirm it works as expected. 

### Are these changes tested?

Yes, the changes are tested. The two tests were added, and the C++ test suite ran. No unexpected failures appeared. 

### Are there any user-facing changes?

Other than newly returning an invalid status when writing after close, no, there should not be any user-facing changes. 

* Closes: apache#35095

Lead-authored-by: Chris Jordan-Squire <chris.jordansquire@gmail.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
…matWriter (apache#37783)

This addresses apacheGH-35095 by adding a flag to IpcFormatWriter to track when a writer has been closed, and check this flag before writes.

### Rationale for this change

This addresses apache#35095 , preventing stream and file IPC writers from writing record batches once the IPC writer has been closed. 

### What changes are included in this PR?

Adding a flag so that an IpcFormatWriter to track when it's been closed, a check before writes in IpcFormatWriter, and two tests to confirm it works as expected. 

### Are these changes tested?

Yes, the changes are tested. The two tests were added, and the C++ test suite ran. No unexpected failures appeared. 

### Are there any user-facing changes?

Other than newly returning an invalid status when writing after close, no, there should not be any user-facing changes. 

* Closes: apache#35095

Lead-authored-by: Chris Jordan-Squire <chris.jordansquire@gmail.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment