added file checking and overwrite functionality to write #94
Conversation
|
Cool. I think one of the strategies often used by apps which save new versions of files (e.g. text processing apps) is to rename the original file to something else, write the new one, and only then remove the original one. That's the only way to ensure that if something goes wrong during the operation (crash, power issue...), you'll end up either with the old or the new file, and not with a single half-corrupt file. Maybe that' strategy could make sense here? I guess renaming an mmapped file and removing it afterwards is OK? |
|
I think it's just a question of how much of this functionality we want to build into Feather.jl itself. Perhaps we could allow users to pass a filename to I think ideally stuff like that would be handled by FileIO (perhaps with us accommodating and interface), though as far as I know it doesn't provide anything like that right now. |
|
@nalimilan , if you have a firm idea of how you'd like to see this implemented, perhaps raise an issue on FileIO? That would potentially provide the functionality for both Feather.jl and CSV.jl all in one go. Though as I recall FileIO wasn't good at supporting all the additional options that are available for CSV.jl. |
|
The problem here is I don't think windows allows you to |
|
That's why you always see things in CSV/Feather tests like obj = nothing; GC.gc(); GC.gc()
rm(filename)For windows, you have to make sure that original mmap-ing gets finalized/GC-ed before |
|
It's taking great self-restraint for me not to share my opinion on windows at this moment. I don't know, how about having this as a linux only option? On windows presumably you can't get the error that instigated this PR since it wouldn't let you write to the file in the first place. |
|
Ok, how about this then? I think this should give pretty reasonable behavior in all 4 cases (whether file exists, OS). |
src/sink.jl
Outdated
| catch e | ||
| @error(string("Unable to delete file $filename. It's possible that it's ", | ||
| "already open and being used by this or another process.")) | ||
| throw(e) |
|
By the way, can we drop 0.6 now? If so, say the word, and I'll take 0.6 stuff out of this PR. |
|
I say we drop 0.6. |
Codecov Report
@@ Coverage Diff @@
## master #94 +/- ##
==========================================
- Coverage 97.22% 95.37% -1.86%
==========================================
Files 4 4
Lines 108 108
==========================================
- Hits 105 103 -2
- Misses 3 5 +2
Continue to review full report at Codecov.
|
|
I removed a bunch of 0.6 stuff. So, if we merge this, we can tag a 1.0-only 0.5 at some point. |
|
The error on appveyor looks like the same 32-bit alignment error we'd been seeing. @quinnj : it'll be up to your vote if we merge this. Yes, the windows behavior isn't great, but it seems to me that this will still be an overall improvement. |
This should fix #93 rather elegantly (thanks to @nalimilan for the suggestion).
Interesting bit of trivia: most of my code which uses
Featherwas actually already doing this outside ofFeather. So, it was rather silly (or cruel) of me to use this as a matter of course but not include this inFeather.jlitself.Note that this makes it so that by default, feather will not allow you to
writeover existing files, but you can passoverwrite=trueto delete the file before writing.