You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by gengliangwang <gi...@git.apache.org> on 2018/10/10 08:48:19 UTC
[GitHub] spark pull request #22684: [SPARK-25699][SQL] Partially push down conjunctiv...
GitHub user gengliangwang opened a pull request:
https://github.com/apache/spark/pull/22684
[SPARK-25699][SQL] Partially push down conjunctive predicated in Orc
## What changes were proposed in this pull request?
Inspired by https://github.com/apache/spark/pull/22574 .
We can partially push down top level conjunctive predicates to Orc.
The code changes include Orc data source under SQL and Hive module.
## How was this patch tested?
New unit test.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gengliangwang/spark pushOrcFilters
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22684.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 #22684
----
commit e2dee098b2efb80238e38068d2ad8681504e03e0
Author: Gengliang Wang <ge...@...>
Date: 2018-10-10T07:00:13Z
push more predicates
commit 798ed5317f953c0aaaf6c5e92126b26f15fe66c0
Author: Gengliang Wang <ge...@...>
Date: 2018-10-10T08:34:34Z
handle hive orc
commit 28322b2efacd5fd1787c0ca1fe64e5db4a5b25fc
Author: Gengliang Wang <ge...@...>
Date: 2018-10-10T08:43:29Z
add jira number in test case
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22684
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3857/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22684
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3843/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by dbtsai <gi...@git.apache.org>.
Github user dbtsai commented on the issue:
https://github.com/apache/spark/pull/22684
Merged into master. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22684
**[Test build #97209 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/97209/testReport)** for PR 22684 at commit [`9d9ed2f`](https://github.com/apache/spark/commit/9d9ed2f8295e2d61ff880860abf0e8551afce04b).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22684
**[Test build #97199 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/97199/testReport)** for PR 22684 at commit [`c2eb87a`](https://github.com/apache/spark/commit/c2eb87ac8310a7045eb55ba7851a38ebf0498300).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22684
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22684
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/97187/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22684: [SPARK-25699][SQL] Partially push down conjunctiv...
Posted by gengliangwang <gi...@git.apache.org>.
Github user gengliangwang commented on a diff in the pull request:
https://github.com/apache/spark/pull/22684#discussion_r224186347
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/orc/OrcFilters.scala ---
@@ -138,39 +138,75 @@ private[sql] object OrcFilters {
dataTypeMap: Map[String, DataType],
expression: Filter,
builder: Builder): Option[Builder] = {
+ createBuilder(dataTypeMap, expression, builder, canPartialPushDownConjuncts = true)
+ }
+
+ /**
+ * @param dataTypeMap a map from the attribute name to its data type.
+ * @param expression the input filter predicates.
+ * @param builder the input SearchArgument.Builder.
+ * @param canPartialPushDownConjuncts whether a subset of conjuncts of predicates can be pushed
+ * down safely. Pushing ONLY one side of AND down is safe to
+ * do at the top level or none of its ancestors is NOT and OR.
+ * @return the builder so far.
+ */
+ private def createBuilder(
+ dataTypeMap: Map[String, DataType],
+ expression: Filter,
+ builder: Builder,
+ canPartialPushDownConjuncts: Boolean): Option[Builder] = {
def getType(attribute: String): PredicateLeaf.Type =
getPredicateLeafType(dataTypeMap(attribute))
import org.apache.spark.sql.sources._
expression match {
case And(left, right) =>
- // At here, it is not safe to just convert one side if we do not understand the
- // other side. Here is an example used to explain the reason.
+ // At here, it is not safe to just convert one side and remove the other side
+ // if we do not understand what the parent filters are.
+ //
+ // Here is an example used to explain the reason.
// Let's say we have NOT(a = 2 AND b in ('1')) and we do not understand how to
// convert b in ('1'). If we only convert a = 2, we will end up with a filter
// NOT(a = 2), which will generate wrong results.
- // Pushing one side of AND down is only safe to do at the top level.
- // You can see ParquetRelation's initializeLocalJobFunc method as an example.
- for {
- _ <- buildSearchArgument(dataTypeMap, left, newBuilder)
- _ <- buildSearchArgument(dataTypeMap, right, newBuilder)
- lhs <- buildSearchArgument(dataTypeMap, left, builder.startAnd())
- rhs <- buildSearchArgument(dataTypeMap, right, lhs)
- } yield rhs.end()
+ //
+ // Pushing one side of AND down is only safe to do at the top level or in the child
+ // AND before hitting NOT or OR conditions, and in this case, the unsupported predicate
+ // can be safely removed.
+ val leftBuilderOption = createBuilder(dataTypeMap, left,
+ newBuilder, canPartialPushDownConjuncts)
+ val rightBuilderOption =
+ createBuilder(dataTypeMap, right, newBuilder, canPartialPushDownConjuncts)
+ (leftBuilderOption, rightBuilderOption) match {
+ case (Some(_), Some(_)) =>
+ for {
+ lhs <- createBuilder(dataTypeMap, left,
+ builder.startAnd(), canPartialPushDownConjuncts)
+ rhs <- createBuilder(dataTypeMap, right, lhs, canPartialPushDownConjuncts)
+ } yield rhs.end()
+
+ case (Some(_), None) if canPartialPushDownConjuncts =>
+ createBuilder(dataTypeMap, left, builder, canPartialPushDownConjuncts)
+
+ case (None, Some(_)) if canPartialPushDownConjuncts =>
+ createBuilder(dataTypeMap, right, builder, canPartialPushDownConjuncts)
+
+ case _ => None
+ }
case Or(left, right) =>
for {
- _ <- buildSearchArgument(dataTypeMap, left, newBuilder)
- _ <- buildSearchArgument(dataTypeMap, right, newBuilder)
- lhs <- buildSearchArgument(dataTypeMap, left, builder.startOr())
- rhs <- buildSearchArgument(dataTypeMap, right, lhs)
+ _ <- createBuilder(dataTypeMap, left, newBuilder, canPartialPushDownConjuncts = false)
+ _ <- createBuilder(dataTypeMap, right, newBuilder, canPartialPushDownConjuncts = false)
+ lhs <- createBuilder(dataTypeMap, left,
+ builder.startOr(), canPartialPushDownConjuncts = false)
+ rhs <- createBuilder(dataTypeMap, right, lhs, canPartialPushDownConjuncts = false)
} yield rhs.end()
case Not(child) =>
for {
- _ <- buildSearchArgument(dataTypeMap, child, newBuilder)
- negate <- buildSearchArgument(dataTypeMap, child, builder.startNot())
+ _ <- createBuilder(dataTypeMap, child, newBuilder, canPartialPushDownConjuncts = false)
+ negate <- createBuilder(dataTypeMap, child, builder.startNot(), false)
--- End diff --
Then it has to be two lines..I am OK with that too.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22684
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22684
**[Test build #97187 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/97187/testReport)** for PR 22684 at commit [`28322b2`](https://github.com/apache/spark/commit/28322b2efacd5fd1787c0ca1fe64e5db4a5b25fc).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22684: [SPARK-25699][SQL] Partially push down conjunctiv...
Posted by dbtsai <gi...@git.apache.org>.
Github user dbtsai commented on a diff in the pull request:
https://github.com/apache/spark/pull/22684#discussion_r224178237
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/orc/OrcFilters.scala ---
@@ -138,39 +138,75 @@ private[sql] object OrcFilters {
dataTypeMap: Map[String, DataType],
expression: Filter,
builder: Builder): Option[Builder] = {
+ createBuilder(dataTypeMap, expression, builder, canPartialPushDownConjuncts = true)
+ }
+
+ /**
+ * @param dataTypeMap a map from the attribute name to its data type.
+ * @param expression the input filter predicates.
+ * @param builder the input SearchArgument.Builder.
+ * @param canPartialPushDownConjuncts whether a subset of conjuncts of predicates can be pushed
+ * down safely. Pushing ONLY one side of AND down is safe to
+ * do at the top level or none of its ancestors is NOT and OR.
+ * @return the builder so far.
+ */
+ private def createBuilder(
+ dataTypeMap: Map[String, DataType],
+ expression: Filter,
+ builder: Builder,
+ canPartialPushDownConjuncts: Boolean): Option[Builder] = {
def getType(attribute: String): PredicateLeaf.Type =
getPredicateLeafType(dataTypeMap(attribute))
import org.apache.spark.sql.sources._
expression match {
case And(left, right) =>
- // At here, it is not safe to just convert one side if we do not understand the
- // other side. Here is an example used to explain the reason.
+ // At here, it is not safe to just convert one side and remove the other side
+ // if we do not understand what the parent filters are.
+ //
+ // Here is an example used to explain the reason.
// Let's say we have NOT(a = 2 AND b in ('1')) and we do not understand how to
// convert b in ('1'). If we only convert a = 2, we will end up with a filter
// NOT(a = 2), which will generate wrong results.
- // Pushing one side of AND down is only safe to do at the top level.
- // You can see ParquetRelation's initializeLocalJobFunc method as an example.
- for {
- _ <- buildSearchArgument(dataTypeMap, left, newBuilder)
- _ <- buildSearchArgument(dataTypeMap, right, newBuilder)
- lhs <- buildSearchArgument(dataTypeMap, left, builder.startAnd())
- rhs <- buildSearchArgument(dataTypeMap, right, lhs)
- } yield rhs.end()
+ //
+ // Pushing one side of AND down is only safe to do at the top level or in the child
+ // AND before hitting NOT or OR conditions, and in this case, the unsupported predicate
+ // can be safely removed.
+ val leftBuilderOption = createBuilder(dataTypeMap, left,
+ newBuilder, canPartialPushDownConjuncts)
+ val rightBuilderOption =
+ createBuilder(dataTypeMap, right, newBuilder, canPartialPushDownConjuncts)
+ (leftBuilderOption, rightBuilderOption) match {
+ case (Some(_), Some(_)) =>
+ for {
+ lhs <- createBuilder(dataTypeMap, left,
+ builder.startAnd(), canPartialPushDownConjuncts)
+ rhs <- createBuilder(dataTypeMap, right, lhs, canPartialPushDownConjuncts)
+ } yield rhs.end()
+
+ case (Some(_), None) if canPartialPushDownConjuncts =>
+ createBuilder(dataTypeMap, left, builder, canPartialPushDownConjuncts)
+
+ case (None, Some(_)) if canPartialPushDownConjuncts =>
+ createBuilder(dataTypeMap, right, builder, canPartialPushDownConjuncts)
+
+ case _ => None
+ }
case Or(left, right) =>
for {
- _ <- buildSearchArgument(dataTypeMap, left, newBuilder)
- _ <- buildSearchArgument(dataTypeMap, right, newBuilder)
- lhs <- buildSearchArgument(dataTypeMap, left, builder.startOr())
- rhs <- buildSearchArgument(dataTypeMap, right, lhs)
+ _ <- createBuilder(dataTypeMap, left, newBuilder, canPartialPushDownConjuncts = false)
+ _ <- createBuilder(dataTypeMap, right, newBuilder, canPartialPushDownConjuncts = false)
+ lhs <- createBuilder(dataTypeMap, left,
+ builder.startOr(), canPartialPushDownConjuncts = false)
+ rhs <- createBuilder(dataTypeMap, right, lhs, canPartialPushDownConjuncts = false)
} yield rhs.end()
case Not(child) =>
for {
- _ <- buildSearchArgument(dataTypeMap, child, newBuilder)
- negate <- buildSearchArgument(dataTypeMap, child, builder.startNot())
+ _ <- createBuilder(dataTypeMap, child, newBuilder, canPartialPushDownConjuncts = false)
+ negate <- createBuilder(dataTypeMap, child, builder.startNot(), false)
--- End diff --
Nit, can you explicitly call `canPartialPushDownConjuncts = false`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22684: [SPARK-25699][SQL] Partially push down conjunctiv...
Posted by dbtsai <gi...@git.apache.org>.
Github user dbtsai commented on a diff in the pull request:
https://github.com/apache/spark/pull/22684#discussion_r224179447
--- Diff: sql/hive/src/main/scala/org/apache/spark/sql/hive/orc/OrcFilters.scala ---
@@ -90,32 +107,51 @@ private[orc] object OrcFilters extends Logging {
expression match {
case And(left, right) =>
- // At here, it is not safe to just convert one side if we do not understand the
- // other side. Here is an example used to explain the reason.
+ // At here, it is not safe to just convert one side and remove the other side
+ // if we do not understand what the parent filters are.
+ //
+ // Here is an example used to explain the reason.
// Let's say we have NOT(a = 2 AND b in ('1')) and we do not understand how to
// convert b in ('1'). If we only convert a = 2, we will end up with a filter
// NOT(a = 2), which will generate wrong results.
- // Pushing one side of AND down is only safe to do at the top level.
- // You can see ParquetRelation's initializeLocalJobFunc method as an example.
- for {
- _ <- buildSearchArgument(dataTypeMap, left, newBuilder)
- _ <- buildSearchArgument(dataTypeMap, right, newBuilder)
- lhs <- buildSearchArgument(dataTypeMap, left, builder.startAnd())
- rhs <- buildSearchArgument(dataTypeMap, right, lhs)
- } yield rhs.end()
+ //
+ // Pushing one side of AND down is only safe to do at the top level or in the child
+ // AND before hitting NOT or OR conditions, and in this case, the unsupported predicate
+ // can be safely removed.
+ val leftBuilderOption = createBuilder(dataTypeMap, left,
+ newBuilder, canPartialPushDownConjuncts)
+ val rightBuilderOption =
+ createBuilder(dataTypeMap, right, newBuilder, canPartialPushDownConjuncts)
+ (leftBuilderOption, rightBuilderOption) match {
--- End diff --
ditto
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22684
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22684: [SPARK-25699][SQL] Partially push down conjunctiv...
Posted by dbtsai <gi...@git.apache.org>.
Github user dbtsai commented on a diff in the pull request:
https://github.com/apache/spark/pull/22684#discussion_r224179579
--- Diff: sql/hive/src/main/scala/org/apache/spark/sql/hive/orc/OrcFilters.scala ---
@@ -90,32 +107,51 @@ private[orc] object OrcFilters extends Logging {
expression match {
case And(left, right) =>
- // At here, it is not safe to just convert one side if we do not understand the
- // other side. Here is an example used to explain the reason.
+ // At here, it is not safe to just convert one side and remove the other side
+ // if we do not understand what the parent filters are.
+ //
+ // Here is an example used to explain the reason.
// Let's say we have NOT(a = 2 AND b in ('1')) and we do not understand how to
// convert b in ('1'). If we only convert a = 2, we will end up with a filter
// NOT(a = 2), which will generate wrong results.
- // Pushing one side of AND down is only safe to do at the top level.
- // You can see ParquetRelation's initializeLocalJobFunc method as an example.
- for {
- _ <- buildSearchArgument(dataTypeMap, left, newBuilder)
- _ <- buildSearchArgument(dataTypeMap, right, newBuilder)
- lhs <- buildSearchArgument(dataTypeMap, left, builder.startAnd())
- rhs <- buildSearchArgument(dataTypeMap, right, lhs)
- } yield rhs.end()
+ //
+ // Pushing one side of AND down is only safe to do at the top level or in the child
+ // AND before hitting NOT or OR conditions, and in this case, the unsupported predicate
+ // can be safely removed.
+ val leftBuilderOption = createBuilder(dataTypeMap, left,
+ newBuilder, canPartialPushDownConjuncts)
+ val rightBuilderOption =
+ createBuilder(dataTypeMap, right, newBuilder, canPartialPushDownConjuncts)
+ (leftBuilderOption, rightBuilderOption) match {
+ case (Some(_), Some(_)) =>
+ for {
+ lhs <- createBuilder(dataTypeMap, left,
+ builder.startAnd(), canPartialPushDownConjuncts)
+ rhs <- createBuilder(dataTypeMap, right, lhs, canPartialPushDownConjuncts)
+ } yield rhs.end()
+
+ case (Some(_), None) if canPartialPushDownConjuncts =>
+ createBuilder(dataTypeMap, left, builder, canPartialPushDownConjuncts)
+
+ case (None, Some(_)) if canPartialPushDownConjuncts =>
+ createBuilder(dataTypeMap, right, builder, canPartialPushDownConjuncts)
+
+ case _ => None
+ }
case Or(left, right) =>
for {
- _ <- buildSearchArgument(dataTypeMap, left, newBuilder)
- _ <- buildSearchArgument(dataTypeMap, right, newBuilder)
- lhs <- buildSearchArgument(dataTypeMap, left, builder.startOr())
- rhs <- buildSearchArgument(dataTypeMap, right, lhs)
+ _ <- createBuilder(dataTypeMap, left, newBuilder, canPartialPushDownConjuncts = false)
+ _ <- createBuilder(dataTypeMap, right, newBuilder, canPartialPushDownConjuncts = false)
+ lhs <- createBuilder(dataTypeMap, left,
+ builder.startOr(), canPartialPushDownConjuncts = false)
+ rhs <- createBuilder(dataTypeMap, right, lhs, canPartialPushDownConjuncts = false)
} yield rhs.end()
case Not(child) =>
for {
- _ <- buildSearchArgument(dataTypeMap, child, newBuilder)
- negate <- buildSearchArgument(dataTypeMap, child, builder.startNot())
+ _ <- createBuilder(dataTypeMap, child, newBuilder, canPartialPushDownConjuncts = false)
+ negate <- createBuilder(dataTypeMap, child, builder.startNot(), false)
--- End diff --
ditto
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22684
**[Test build #97187 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/97187/testReport)** for PR 22684 at commit [`28322b2`](https://github.com/apache/spark/commit/28322b2efacd5fd1787c0ca1fe64e5db4a5b25fc).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22684
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22684: [SPARK-25699][SQL] Partially push down conjunctiv...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/22684
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22684
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/97209/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22684
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3851/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22684
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22684: [SPARK-25699][SQL] Partially push down conjunctiv...
Posted by dbtsai <gi...@git.apache.org>.
Github user dbtsai commented on a diff in the pull request:
https://github.com/apache/spark/pull/22684#discussion_r224174206
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/orc/OrcFilters.scala ---
@@ -138,39 +138,75 @@ private[sql] object OrcFilters {
dataTypeMap: Map[String, DataType],
expression: Filter,
builder: Builder): Option[Builder] = {
+ createBuilder(dataTypeMap, expression, builder, canPartialPushDownConjuncts = true)
+ }
+
+ /**
+ * @param dataTypeMap a map from the attribute name to its data type.
+ * @param expression the input filter predicates.
+ * @param builder the input SearchArgument.Builder.
+ * @param canPartialPushDownConjuncts whether a subset of conjuncts of predicates can be pushed
+ * down safely. Pushing ONLY one side of AND down is safe to
+ * do at the top level or none of its ancestors is NOT and OR.
+ * @return the builder so far.
+ */
+ private def createBuilder(
+ dataTypeMap: Map[String, DataType],
+ expression: Filter,
+ builder: Builder,
+ canPartialPushDownConjuncts: Boolean): Option[Builder] = {
def getType(attribute: String): PredicateLeaf.Type =
getPredicateLeafType(dataTypeMap(attribute))
import org.apache.spark.sql.sources._
expression match {
case And(left, right) =>
- // At here, it is not safe to just convert one side if we do not understand the
- // other side. Here is an example used to explain the reason.
+ // At here, it is not safe to just convert one side and remove the other side
+ // if we do not understand what the parent filters are.
+ //
+ // Here is an example used to explain the reason.
// Let's say we have NOT(a = 2 AND b in ('1')) and we do not understand how to
// convert b in ('1'). If we only convert a = 2, we will end up with a filter
// NOT(a = 2), which will generate wrong results.
- // Pushing one side of AND down is only safe to do at the top level.
- // You can see ParquetRelation's initializeLocalJobFunc method as an example.
- for {
- _ <- buildSearchArgument(dataTypeMap, left, newBuilder)
- _ <- buildSearchArgument(dataTypeMap, right, newBuilder)
- lhs <- buildSearchArgument(dataTypeMap, left, builder.startAnd())
- rhs <- buildSearchArgument(dataTypeMap, right, lhs)
- } yield rhs.end()
+ //
+ // Pushing one side of AND down is only safe to do at the top level or in the child
+ // AND before hitting NOT or OR conditions, and in this case, the unsupported predicate
+ // can be safely removed.
+ val leftBuilderOption = createBuilder(dataTypeMap, left,
+ newBuilder, canPartialPushDownConjuncts)
+ val rightBuilderOption =
+ createBuilder(dataTypeMap, right, newBuilder, canPartialPushDownConjuncts)
--- End diff --
Can you make the format the same as `leftBuilderOption`? Also, add another empty line before `(leftBuilderOption, rightBuilderOption)`. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22684: [SPARK-25699][SQL] Partially push down conjunctiv...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22684#discussion_r224005596
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/orc/OrcFilterSuite.scala ---
@@ -382,5 +382,40 @@ class OrcFilterSuite extends OrcTest with SharedSQLContext {
))
)).get.toString
}
+
+ // Can not remove unsupported `StringContains` predicate since it is under `Or` operator.
+ assert(OrcFilters.createFilter(schema, Array(
+ Or(
+ LessThan("a", 10),
+ And(
+ StringContains("b", "prefix"),
+ GreaterThan("a", 1)
+ )
+ )
+ )).isEmpty)
+
+ // Safely remove unsupported `StringContains` predicate and push down `LessThan`
+ assertResult("leaf-0 = (LESS_THAN a 10), expr = leaf-0") {
+ OrcFilters.createFilter(schema, Array(
+ And(
+ LessThan("a", 10),
+ StringContains("b", "prefix")
+ )
+ )).get.toString
+ }
+
+ // Safely remove unsupported `StringContains` predicate and push down `LessThan`
--- End diff --
There is another predicate pushed down. We shall mention it too?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22684
**[Test build #97209 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/97209/testReport)** for PR 22684 at commit [`9d9ed2f`](https://github.com/apache/spark/commit/9d9ed2f8295e2d61ff880860abf0e8551afce04b).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22684
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by dbtsai <gi...@git.apache.org>.
Github user dbtsai commented on the issue:
https://github.com/apache/spark/pull/22684
LGTM. Just some styling feedback. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by gengliangwang <gi...@git.apache.org>.
Github user gengliangwang commented on the issue:
https://github.com/apache/spark/pull/22684
@dbtsai @gatorsmile
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22684
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/97199/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22684: [SPARK-25699][SQL] Partially push down conjunctive predi...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22684
**[Test build #97199 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/97199/testReport)** for PR 22684 at commit [`c2eb87a`](https://github.com/apache/spark/commit/c2eb87ac8310a7045eb55ba7851a38ebf0498300).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org