ISSUE 86: Creating Hadoop FS based on URI and conf #87
Conversation
Added static getFS to check for null and non-null default URI Added basic test on file:// (couldn't add s3a due to credentials and aws classes)
sadikovi
left a comment
There was a problem hiding this comment.
Could you address the comments? Otherwise, it looks good.
| override def compute(split: Partition, context: TaskContext): Iterator[ParquetFileStatus] = { | ||
| val configuration = hadoopConfiguration | ||
| val fs = FileSystem.get(configuration) | ||
| val fs = ParquetStatisticsRDD.getFS(configuration) |
There was a problem hiding this comment.
I'd prefer getFileSystem method name, but it does not matter that much, so it is up to you - feel free to change or keep the current name.
| } | ||
|
|
||
| /** | ||
| * Get FileSystem based on URI |
There was a problem hiding this comment.
You should add something like that: Returns file system for FILTER_DIR path, if provided, otherwise, the default file system based on Hadoop configuration.
| def getFS(configuration: Configuration): FileSystem = { | ||
| val filterURI = configuration.get(ParquetMetastoreSupport.FILTER_DIR) | ||
| filterURI match { | ||
| case uri:Any => FileSystem.get(new URI(uri), configuration) |
There was a problem hiding this comment.
There is an unnecessary space after =>.
| val filterURI = configuration.get(ParquetMetastoreSupport.FILTER_DIR) | ||
| filterURI match { | ||
| case uri:Any => FileSystem.get(new URI(uri), configuration) | ||
| case _ => FileSystem.get(configuration) |
There was a problem hiding this comment.
Unnecessary spaces after _ and =>.
| * Get FileSystem based on URI | ||
| */ | ||
| def getFS(configuration: Configuration): FileSystem = { | ||
| val filterURI = configuration.get(ParquetMetastoreSupport.FILTER_DIR) |
There was a problem hiding this comment.
I'd change this method to something like this:
// Returns null if option is not set in configuration
Option(configuration.get(ParquetMetastoreSupport.FILTER_DIR)) match {
case Some(uri) => FileSystem.get(new URI(uri), configuration)
case None => FileSystem.get(configuration)
}| // and also AWS SDK classes in classpath | ||
| hadoopConf.set(ParquetMetastoreSupport.FILTER_DIR, "file://issue-86-test/index_metastore") | ||
| val fs = ParquetStatisticsRDD.getFS(hadoopConf) | ||
| fs shouldBe a [FileSystem] |
There was a problem hiding this comment.
It will always be a FileSystem. I think it would be better to check fs.getScheme() should be "file" - I don't quite remember the value for local file system:).
And another test (or extend this test) is setting the default file system to local, but unset FILTER_DIR config, so that we test another branch of the code.
|
Thanks for the comments Ivan. Incorporated all your review comments. |
|
Merged, thanks. |
Added static getFS to check for null and non-null default URI
Added basic test on file:// (couldn't add s3a due to credentials and aws classes)