You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by saucam <gi...@git.apache.org> on 2015/01/28 11:59:14 UTC
[GitHub] spark pull request: [SQL][SPARK-5453] Use property 'mapreduce.inpu...
GitHub user saucam opened a pull request:
https://github.com/apache/spark/pull/4246
[SQL][SPARK-5453] Use property 'mapreduce.input.pathFilter.class' to set a custom filter class for input files
This PR adds support for using a custom filter class for input files for queries. We can re-use the existing property in hive-site.xml for this.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/saucam/spark hive_site
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/4246.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #4246
----
commit 53e86c888909399992f40502ab1c81647e321ba8
Author: Yash Datta <ya...@guavus.com>
Date: 2015-01-28T10:43:21Z
SPARK-5453: Use property 'mapreduce.input.pathFilter.class' to set a custom filter class for input files
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SQL][SPARK-5453] Use property 'mapreduce.inpu...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/4246
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SQL][SPARK-5453] Use property 'mapreduce.inpu...
Posted by saucam <gi...@git.apache.org>.
Github user saucam commented on the pull request:
https://github.com/apache/spark/pull/4246#issuecomment-91868767
please retest
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SQL][SPARK-5453] Use property 'mapreduce.inpu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4246#issuecomment-91889224
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/30080/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SQL][SPARK-5453] Use property 'mapreduce.inpu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4246#issuecomment-90353017
[Test build #29769 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29769/consoleFull) for PR 4246 at commit [`2bab8d3`](https://github.com/apache/spark/commit/2bab8d392695d61adcbf8fc7a5c3d24bc770ae6a).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SQL][SPARK-5453] Use property 'mapreduce.inpu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4246#issuecomment-90353023
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29769/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SQL][SPARK-5453] Use property 'mapreduce.inpu...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/4246#discussion_r27848179
--- Diff: core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala ---
@@ -201,6 +201,22 @@ class SparkHadoopUtil extends Logging {
val baseStatus = fs.getFileStatus(basePath)
if (baseStatus.isDir) recurse(basePath) else Array(baseStatus)
}
+
+ /**
+ * Get [[FileStatus]] objects for all leaf children (files) under the given base path. If the
+ * given path points to a file, return a single-element collection containing [[FileStatus]] of
+ * that file. Uses pathFilter to filter out certain filestatuses
+ */
+ def listLeafStatusesFiltered(fs: FileSystem, basePath: Path,
+ filter: Option[PathFilter]): Seq[FileStatus] = {
--- End diff --
Indent as follows: https://cwiki.apache.org/confluence/display/SPARK/Spark+Code+Style+Guide
```scala
def listLeafStatusesFiltered(
fs: FileSystem,
basePath: Path,
filter: Option[PathFilter]): Seq[FileStatus] = {
...
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SQL][SPARK-5453] Use property 'mapreduce.inpu...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/4246#issuecomment-90322460
ok to test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SQL][SPARK-5453] Use property 'mapreduce.inpu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4246#issuecomment-91889191
[Test build #30080 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/30080/consoleFull) for PR 4246 at commit [`329445b`](https://github.com/apache/spark/commit/329445bc2bd4eb84d425c9466b912584a712d8b7).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SQL][SPARK-5453] Use property 'mapreduce.inpu...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/4246#discussion_r27848210
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLContext.scala ---
@@ -162,6 +166,26 @@ class SQLContext(@transient val sparkContext: SparkContext)
@transient
protected[sql] val cacheManager = new CacheManager(this)
+ private val DEFAULT_FILTER: PathFilter = new PathFilter() {
+ @Override
+ def accept(file: Path): Boolean = {
+ return true
+ }
+ }
+
+ /**
+ * Return PathFilter to be used for filtering input files using custom filter class
+ */
+ protected[sql] def getPathFilter(): Option[PathFilter] = {
--- End diff --
Why is this an option if it always returns `Some`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SQL][SPARK-5453] Use property 'mapreduce.inpu...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/4246#issuecomment-91941342
Thanks for addressing the comments. As I look at this closer I wonder if we aren't approaching this in the wrong way. If this is a hadoop option that FileInputFormat supports, why aren't users just setting it in the hadoop configuration? (i.e. `sc.hadoopConfiguration.set("mapreduce.input.pathFilter", ...)`). As it stands now, it is sprinkling new logic all over that is reading from some random hadoop config. This seems very likely to lead to errors in the future. If we aren't properly propagating the hadoop config that is another thing that should be fixed separately.
Unless there is something that I am missing, I suggest that we close this issue and take the hadoop configuration approach.
If you do think this is the right approach, we can discuss further, but I have some feeback on the PR in general:
- The indentation is still off in several places. Please look at the coding style guide.
- There is a lot of duplicated logic (some of which is wrong). It would be better to isolate these things, so if we need to fix it in the future we can. For example I see this patter a lot:
```scala
val filterClassName = sqlContext.getConf(NewFileInputFormat.PATHFILTER_CLASS, "")
if (filterClassName.nonEmpty) {
val filterClazz = Class.forName(filterClassName).asInstanceOf[Class[_ <: PathFilter]]
```
However `Class.forName` is probably not using a classloader that can see user code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SQL][SPARK-5453] Use property 'mapreduce.inpu...
Posted by saucam <gi...@git.apache.org>.
Github user saucam commented on the pull request:
https://github.com/apache/spark/pull/4246#issuecomment-91996558
hey @marmbrus , this property is needed because :
1. The 'hijacked' parquet read path would not use mapreduce property while reading schema/footers,
see refresh method in ParquetRelation2.scala
2. HiveTableScan would need the pathfilter while creating RDD
see makeRDDForPartitionedTable in TableReader.scala
These are not using the mapreduce.input.pathFilter (even if its is set by the user) property and hence the extra code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SQL][SPARK-5453] Use property 'mapreduce.inpu...
Posted by saucam <gi...@git.apache.org>.
Github user saucam commented on the pull request:
https://github.com/apache/spark/pull/4246#issuecomment-89779951
Thanks for the suggestions @marmbrus , I have refactored PathFilter creation in SQLContext. Covered more instances of listStatus. please review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SQL][SPARK-5453] Use property 'mapreduce.inpu...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/4246#issuecomment-89087051
Sorry for the delay reviewing this. At a high level this seems like a reasonable thing to support. I'd suggest that we consolidate the logic for constructing the `Option[PathFilter]` into a single location in `SQLConf` instead of duplicating it. We should also document that we support his property in the programming guide and audit the code base to make sure we aren't missing any places where we list files.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SQL][SPARK-5453] Use property 'mapreduce.inpu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4246#issuecomment-91868956
[Test build #30080 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/30080/consoleFull) for PR 4246 at commit [`329445b`](https://github.com/apache/spark/commit/329445bc2bd4eb84d425c9466b912584a712d8b7).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SQL][SPARK-5453] Use property 'mapreduce.inpu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4246#issuecomment-71816217
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SQL][SPARK-5453] Use property 'mapreduce.inpu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4246#issuecomment-90322695
[Test build #29769 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29769/consoleFull) for PR 4246 at commit [`2bab8d3`](https://github.com/apache/spark/commit/2bab8d392695d61adcbf8fc7a5c3d24bc770ae6a).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org