GH-3279: Introduce StorageProvider to de-couple Hadoop vs NIO constructs #3280
+412
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
ArnavBalyan
commented
Aug 25, 2025
ArnavBalyan
commented
- Add a minimal StorageProvider abstraction and selector that routes to hadoop vs non-hadoop classes.
- Make Hadoop I/O resolve FileSystem per path to correctly hit the right connector.
- This isolates local I/O from Hadoop today and sets up a clean interface to pull the correct concrete implementation at runtime.
shangxinli
reviewed
Aug 25, 2025
| * Opens the given path for reading. | ||
| * | ||
| * @param path fully-qualified file path (implementation specific semantics) | ||
| * @return an InputStream that must be closed by the caller |
There was a problem hiding this comment.
The JavaDoc should specify what specific IOException
subclasses might be thrown (e.g., FileNotFoundException,
AccessDeniedException) to help implementers and users handle
errors appropriately.
shangxinli
reviewed
Aug 25, 2025
| * @param path fully-qualified file path | ||
| * @param overwrite whether an existing file should be replaced | ||
| * @return an OutputStream that must be closed by the caller | ||
| */ |
There was a problem hiding this comment.
The interface should clarify stream ownership and closing
responsibilities. Consider returning AutoCloseable wrappers or
documenting that callers must use try-with-resources.
jerolba
reviewed
Aug 26, 2025
| * @param path fully-qualified file path (implementation specific semantics) | ||
| * @return an InputStream that must be closed by the caller | ||
| */ | ||
| InputStream openForRead(String path) throws IOException; |
There was a problem hiding this comment.
In the future if we want to use this abstraction to read parquet files, the code requires to create a SeekableInputStream
Can be more useful to use SeekableInputStream that already extends InputStream?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.