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

[Integration][Go] IPC files do not embed a valid IPC stream #43837

Closed
bkietz opened this issue Aug 26, 2024 · 2 comments · Fixed by #43890
Closed

[Integration][Go] IPC files do not embed a valid IPC stream #43837

bkietz opened this issue Aug 26, 2024 · 2 comments · Fixed by #43890

Comments

@bkietz
Copy link
Member

bkietz commented Aug 26, 2024

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

The format guarantees that each IPC file embeds a valid IPC stream in order to allow readers to ignore the Footer, skip the file's leading magic, and reuse a stream reader.

#43834 adds validation of the embedded IPC stream and files written by Go fail. When writing IPC files Go seems to omit the stream's EOS so that a stream-only reader will attempt to read the Footer as a Message.

Component(s)

Go, Integration

@joellubi
Copy link
Member

Thanks for surfacing this issue @bkietz, I can work on the fix.

@joellubi
Copy link
Member

Issue resolved by pull request 43890
#43890

mapleFU pushed a commit to mapleFU/arrow that referenced this issue Sep 3, 2024
…suring that EOS indicator is written in file (apache#43890)

### Rationale for this change

Fixes: apache#43837

Much of the logic between the ipc stream writer and the file writer was split. This PR changes the file writer so that it uses a stream writer internally, ensuring that a valid stream is embedded within the file.

**TODO**
- [x] Remove @ bkietz's commits

### What changes are included in this PR?

- Refactor `fileWriter` to embed `streamWriter` and defer relevant methods
- Add test

### Are these changes tested?

Yes

### Are there any user-facing changes?

Go-generated IPC files will contain the EOS indicator

* GitHub Issue: apache#43837

Authored-by: Joel Lubinitsky <[email protected]>
Signed-off-by: Joel Lubinitsky <[email protected]>
zanmato1984 pushed a commit to zanmato1984/arrow that referenced this issue Sep 6, 2024
…suring that EOS indicator is written in file (apache#43890)

### Rationale for this change

Fixes: apache#43837

Much of the logic between the ipc stream writer and the file writer was split. This PR changes the file writer so that it uses a stream writer internally, ensuring that a valid stream is embedded within the file.

**TODO**
- [x] Remove @ bkietz's commits

### What changes are included in this PR?

- Refactor `fileWriter` to embed `streamWriter` and defer relevant methods
- Add test

### Are these changes tested?

Yes

### Are there any user-facing changes?

Go-generated IPC files will contain the EOS indicator

* GitHub Issue: apache#43837

Authored-by: Joel Lubinitsky <[email protected]>
Signed-off-by: Joel Lubinitsky <[email protected]>
khwilson pushed a commit to khwilson/arrow that referenced this issue Sep 14, 2024
…suring that EOS indicator is written in file (apache#43890)

### Rationale for this change

Fixes: apache#43837

Much of the logic between the ipc stream writer and the file writer was split. This PR changes the file writer so that it uses a stream writer internally, ensuring that a valid stream is embedded within the file.

**TODO**
- [x] Remove @ bkietz's commits

### What changes are included in this PR?

- Refactor `fileWriter` to embed `streamWriter` and defer relevant methods
- Add test

### Are these changes tested?

Yes

### Are there any user-facing changes?

Go-generated IPC files will contain the EOS indicator

* GitHub Issue: apache#43837

Authored-by: Joel Lubinitsky <[email protected]>
Signed-off-by: Joel Lubinitsky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
@bkietz @joellubi and others