Support java.nio.Path in HDF5File - #8
Conversation
| */ | ||
| public HDF5File(final File file) { | ||
| this(file, OpenMode.READ_ONLY); | ||
| this(file.toPath()); |
There was a problem hiding this comment.
This should have a check for null here to avoid NPR. ex
this(file == null ? null : file.toPath());
| * @deprecated use {#getPath()}. | ||
| */ | ||
| @Deprecated | ||
| public Path getPath() { |
There was a problem hiding this comment.
you've accidentally deprecated this
| Files.createFile(file); | ||
| } | ||
| fileId = H5.H5Fopen(file.getAbsolutePath(), mode.getFlags(), HDF5Constants.H5P_DEFAULT); | ||
| fileId = H5.H5Fopen(file.toAbsolutePath().toString(), mode.getFlags(), HDF5Constants.H5P_DEFAULT); |
There was a problem hiding this comment.
So this will work for Paths that are actually files on a local filesystem. It will fail for a Path that is something more abstract, like a reference to an object in google's object store or a file stored in hdfs. The main reason for us to switch to path was to support these sorts of non-file data storage containers. I think if we want to support paths here, we'll need to A) check if the path is actually a local file and B) download and cache a non-local file somewhere on the local filesystem if it is not in order to provide a local file path.
I think I would support a change that would do that with appropriate logging messages to help users understand what's happening, but just changing the type to Path without addressing the issue seems like a bad idea to me. If the caller knows their path is actually a file they can always do the path-> file conversion themselves.
There was a problem hiding this comment.
Do you mean to copy to a temp directory the HDFS/GCS file and read from there? In that case, how the READ_WRITE option should work? Two ideas about that, but with some problems for both:
- Write only the temp file, and copy back to the "original"
Pathon close. This will not update the provided path until the HDF5File is closed, so that means that changes made by the API won't be reflected if the file is open with a second holder. - Every time that a dataset is added to the temp file, copy back to the "original" Path. This will have performance issues unless we can append just the generated data.
Waiting for your feedback on how to proceed.
There was a problem hiding this comment.
That's exactly what I was thinking, but I hadn't thought through how writing would work.
I think the second option is probably not feasible. Identifying when the temp file has been updated and copying it back to the original path would be tricky and poorly performing I think.
The first suggestion seems reasonable but it's definitely going to be error prone. People are not always good about closing files. We'd have to have very clear logging explaining what's going on and aggressively fail if a file fails to be written back correctly.
What would you think about supporting Paths only for the read only case and throwing an explanatory error if someone tries to use a non-file path with write enabled?
There was a problem hiding this comment.
I would like to have support to write HD5 files too, but it is true that it could be very messy if it is on close. I will think about it and come back to the PR if I could find a solution...if not, I will add the cached-reading for having at least some support...
1 similar comment
3 participants
No description provided.