Uh oh!
There was an error while loading. Please reload this page.
PARQUET-674: Add InputFile abstraction for openable files. - #368
Conversation
rdblue
commented
Sep 12, 2016
@julienledem and @robert3005, we might want to get this in 1.9.0 (though this is a low priority). It adds |
3bb875c to
f677944Comparerobert3005
commented
Sep 13, 2016
👍 That looks like a good abstraction in general. Thanks! |
| import java.util.concurrent.Executors; | ||
| import java.util.concurrent.Future; | ||
| import org.apache.commons.math3.analysis.function.Add; |
There was a problem hiding this comment.
Ugh, I hate how IntelliJ now adds static imports automatically. It constantly adds these when I pause typing a name. Thanks for catching it!
f677944 to
4a7c327Compare| /** | ||
| * Returns the file location. | ||
| */ | ||
| String getLocation(); |
There was a problem hiding this comment.
No, this is an abstraction that isn't tied to Hadoop or another FS library. A string location should be portable across implementations.
There was a problem hiding this comment.
Yeah I was thinking more on the lines of a java Path / URI.
There was a problem hiding this comment.
do we need the location at all? Is this for showing in error messages? Maybe just toString is enough?
piyushnarang
commented
Sep 13, 2016
👍 |
rdblue
commented
Sep 17, 2016
@julienledem, could you take a look at this? It would be better to do this for 1.9.0 than to do it later because it would prevent exposing a public method. Thanks! |
julienledem
left a comment
There was a problem hiding this comment.
overall this looks good to me. I made some comments
| * {@code ParquetDataSource} is an interface with the methods needed by Parquet | ||
| * to read data files using {@link SeekableInputStream} instances. | ||
| */ | ||
| public interface ParquetDataSource { |
There was a problem hiding this comment.
It's a SeekableInputStream provider with a length.
maybe call it InputFile ?
There was a problem hiding this comment.
Yeah, I wasn't too happy with the name either. InputFile is something I hadn't though of and sounds pretty good. I'll go with that.
| import org.apache.parquet.io.ParquetDataSource; | ||
| import java.io.IOException; | ||
| public class HadoopDataSource implements ParquetDataSource, Configurable { |
There was a problem hiding this comment.
Do we need to make it Configurable?
The conf is passed in the constructor and does not need to be settable or exposed.
even better once initialized, the conf is not used anymore. I would remove the conf field as well.
There was a problem hiding this comment.
This was to be able to create a ParquetMetadataConverter using its constructor that takes a Configuration, but I think it's better to remove this because that option should be removed in the next release.
rdblue
commented
Oct 3, 2016
@julienledem, thanks for your comments. I implemented your suggestions so I think this is about ready when tests are passing. |
julienledem
commented
Oct 3, 2016
+1 |
Author: Ryan Blue <blue@apache.org> Closesapache#368 from rdblue/PARQUET-674-add-data-source and squashes the following commits: 8c689e9 [Ryan Blue] PARQUET-674: Implement review comments. 4a7c327 [Ryan Blue] PARQUET-674: Add DataSource abstraction for openable files.
Author: Ryan Blue <blue@apache.org> Closesapache#368 from rdblue/PARQUET-674-add-data-source and squashes the following commits: 8c689e9 [Ryan Blue] PARQUET-674: Implement review comments. 4a7c327 [Ryan Blue] PARQUET-674: Add DataSource abstraction for openable files.
Author: Ryan Blue <blue@apache.org> Closesapache#368 from rdblue/PARQUET-674-add-data-source and squashes the following commits: 8c689e9 [Ryan Blue] PARQUET-674: Implement review comments. 4a7c327 [Ryan Blue] PARQUET-674: Add DataSource abstraction for openable files.
Author: Ryan Blue <blue@apache.org> Closesapache#368 from rdblue/PARQUET-674-add-data-source and squashes the following commits: 8c689e9 [Ryan Blue] PARQUET-674: Implement review comments. 4a7c327 [Ryan Blue] PARQUET-674: Add DataSource abstraction for openable files.
No description provided.