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

SerializedFileWriter comments about multiple call on consumed self #2935

Closed
pier-oliviert opened this issue Oct 26, 2022 · 3 comments · Fixed by #2938
Closed

SerializedFileWriter comments about multiple call on consumed self #2935

pier-oliviert opened this issue Oct 26, 2022 · 3 comments · Fixed by #2938
Labels
parquet Changes to the parquet crate question Further information is requested

Comments

@pier-oliviert
Copy link
Contributor

Which part is this question about

I was reading comments surrounding the SerializedFileWriter code as I was implementing the writer on a codebase I'm working on and noticed this comment:

Can be called multiple times. It is up to implementation to either result in
no-op, or return an Err for subsequent calls.

https://github.com/apache/arrow-rs/blob/master/parquet/src/file/writer.rs#L180-L181

Describe your question

The code itself is clear enough to understand what is going on but I noted that the signature consumes self which makes me think that it's not really possible, in general, to call this method twice since self will be consumed and is not included in FileMetaData.

Should the comment be modified? Or am I missing something else with regards to the writer.

@pier-oliviert pier-oliviert added the question Further information is requested label Oct 26, 2022
@tustvold
Copy link
Contributor

Aah, thank you yeah this is out of date. The writer was changed to be consuming in #1719

tustvold added a commit to tustvold/arrow-rs that referenced this issue Oct 26, 2022
tustvold added a commit to tustvold/arrow-rs that referenced this issue Oct 26, 2022
@pier-oliviert
Copy link
Contributor Author

Thank you for creating the PR @tustvold! I was going to do it after confirmation, you beat me to it!

@alamb alamb added the parquet Changes to the parquet crate label Oct 28, 2022
@alamb
Copy link
Contributor

alamb commented Oct 28, 2022

label_issue.py automatically added labels {'parquet'} from #2938

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parquet Changes to the parquet crate question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants