Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

FileSink now closes the underlying writer. #1213

Merged
merged 1 commit into from
Aug 5, 2022

Conversation

samkaufman
Copy link
Contributor

@samkaufman samkaufman commented Aug 5, 2022

This PR modifies FileSink to, on close, close its inner FileStreamer's owned writer. At the moment, the inner AsyncWrite's close method is not called when the FileSink is closed.

This is a minor backwards-incompatible change. While it won't affect cases where the FileSink wraps an actual file, which is generally closed when dropped, it may affect behavior when wrapping an AsyncWrite which expects close to be called. (In my case, that was a handle to an S3 object.)

@jorgecarleitao jorgecarleitao added the bug Something isn't working label Aug 5, 2022
@samkaufman samkaufman changed the title FileSink now closes the underlying writer. FileSink now closes the underlying writer. Aug 5, 2022
@codecov
Copy link

codecov bot commented Aug 5, 2022

Codecov Report

Merging #1213 (90d1193) into main (3b9d86b) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1213      +/-   ##
==========================================
+ Coverage   83.48%   83.50%   +0.02%     
==========================================
  Files         356      356              
  Lines       37081    37082       +1     
==========================================
+ Hits        30957    30967      +10     
+ Misses       6124     6115       -9     
Impacted Files Coverage Δ
src/io/parquet/write/sink.rs 72.41% <100.00%> (+0.23%) ⬆️
src/bitmap/utils/slice_iterator.rs 97.56% <0.00%> (-1.22%) ⬇️
src/io/ipc/read/reader.rs 96.32% <0.00%> (ø)
src/array/utf8/mod.rs 83.64% <0.00%> (+0.92%) ⬆️
src/array/binary/mod.rs 90.12% <0.00%> (+1.23%) ⬆️
src/io/ipc/read/array/boolean.rs 98.14% <0.00%> (+7.40%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@samkaufman
Copy link
Contributor Author

samkaufman commented Aug 5, 2022

The "Integration IPC / Flight / Test" failure seems to be a result of this recent commit: apache/arrow@6d1bc62. Perhaps merge should hold off until that's resolved (by a separate PR?).

@samkaufman samkaufman marked this pull request as ready for review August 5, 2022 23:04
@jorgecarleitao jorgecarleitao merged commit 80de3d2 into jorgecarleitao:main Aug 5, 2022
@jorgecarleitao
Copy link
Owner

Thanks a lot @samkaufman 🙇 Yes, ignore the integration test - I need to fix that as something upstream changed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants