-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add InMemoryFileIO as a test helper class #6538
Add InMemoryFileIO as a test helper class #6538
Conversation
InMemoryInputFile as a test helper class stitching the two together and maintaining an in-memory listing of files. Add a dedicated unittest for the new test helper.
As suggested in #6428 (comment) this PR breaks out the addition of the InMemoryFileIO as a separate addition that can be generally useful in iceberg-core for unittesting purposes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall LGTM, just a few small suggestions
|
||
public class InMemoryFileIO implements FileIO { | ||
|
||
private Map<String, byte[]> inMemoryFiles = Maps.newHashMap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe worth using Maps.newConcurrentMap()
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
private Map<String, byte[]> inMemoryFiles = Maps.newHashMap(); | ||
private boolean closed = false; | ||
|
||
public void addFile(String path, byte[] contents) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: in other FileIO implementations we use location
rather than path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
public void deleteFile(String path) { | ||
Preconditions.checkState(!closed, "Cannot call deleteFile after calling close()"); | ||
if (!inMemoryFiles.containsKey(path)) { | ||
throw new NotFoundException("No in-memory file found for path: %s", path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: in other FileIO implementations we use location
rather than path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@Override | ||
public void deleteFile(String path) { | ||
Preconditions.checkState(!closed, "Cannot call deleteFile after calling close()"); | ||
if (!inMemoryFiles.containsKey(path)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: what about using the result of the remove
call here? if (!inMemoryFiles.remove(path)) { ... }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, also updated the get
in newInputFile
to follow the same pattern.
Thanks! Suggestions applied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me!
Thanks @dennishuo !! |
Add InMemoryFileIO alongside existing InMemoryOutputFile and InMemoryInputFile as a test helper class stitching the two together and maintaining an in-memory listing of files. Add a dedicated unittest for the new test helper.