[GitHub] [parquet-mr] belugabehr edited a comment on pull request #815: PARQUET-1776: NIO wrapper for Output/Input File

[GitBox <gi...@apache.org>]

belugabehr edited a comment on pull request #815:
URL: https://github.com/apache/parquet-mr/pull/815#issuecomment-696754782


   @HunterL 
   
   OK, so I've been recently diving into Java NIO a bit and let me add some thoughts that relate to this task:
   
   Please change the naming convention and drop the "Local" prefix.  I think Nio is probably best.  As an example of why this matters:
   
   ```
   // This exists in the current PR
   return new LocalSeekableInputStream(Files.newInputStream(path));
   ```
   
   Well that statement `Files.newInputStream(path)` will provide a new InputStream, but the InputStream is totally driven by whatever the path points at.  Things within the JVM could be setup to allow the provided path to point at an FTP location for example and that would be a _remote_ read and not a _local_ read.
   
   
   I also think you will have better luck storing, internal to the Input/Output streams a `SeekableByteChannel` instead of an `OutputStream` or an `InputStream`.  This will allow you to easily track the position aspect of the underlying stream without having to do it yourself.  For example, the current `OutputStream` implementation does not currently implement this feature:
   
   ```
     @Override
     public long getPos() throws IOException {
       return 0;
     }
   ```
   
   Thanks.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org