You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by maropu <gi...@git.apache.org> on 2018/09/14 02:29:43 UTC
[GitHub] spark pull request #22417: [SPARK-25426][SQL] Handles subexpression eliminat...
GitHub user maropu opened a pull request:
https://github.com/apache/spark/pull/22417
[SPARK-25426][SQL] Handles subexpression elimination config inside CodeGeneratorWithInterpretedFallback
## What changes were proposed in this pull request?
This pr handled the subexpression elimination config inside `CodeGeneratorWithInterpretedFallback`, and then removed the duplicate fallback logic in `UnsafeProjection`.
This pr comes from #22355.
## How was this patch tested?
Added tests in `CodeGeneratorWithInterpretedFallbackSuite`.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/maropu/spark SPARK-25426
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22417.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 #22417
----
commit 35e7911958d5be8d7b66803ba7e11cd22bc4bbe5
Author: Takeshi Yamamuro <ya...@...>
Date: 2018-09-14T02:23:30Z
Fix
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22417: [SPARK-25426][SQL] Handles subexpression eliminat...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/22417#discussion_r217667918
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala ---
@@ -59,6 +59,6 @@ abstract class CodeGeneratorWithInterpretedFallback[IN, OUT] extends Logging {
}
}
- protected def createCodeGeneratedObject(in: IN): OUT
+ protected def createCodeGeneratedObject(in: IN, subexpressionEliminationEnabled: Boolean): OUT
--- End diff --
ah, yes. Looks reasonable. I'll update soon.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22417: [SPARK-25426][SQL] Handles subexpression elimination con...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22417
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96058/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22417: [SPARK-25426][SQL] Handles subexpression elimination con...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22417
**[Test build #96073 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96073/testReport)** for PR 22417 at commit [`ea7e473`](https://github.com/apache/spark/commit/ea7e47350883d7fb18318e0040a5492dc4a4aa07).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22417: [SPARK-25426][SQL] Handles subexpression elimination con...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22417
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/3101/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22417: [SPARK-25426][SQL] Handles subexpression elimination con...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22417
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 #22417: [SPARK-25426][SQL] Remove the duplicate fallback logic i...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/22417
LGTM too.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22417: [SPARK-25426][SQL] Remove the duplicate fallback logic i...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22417
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/3114/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22417: [SPARK-25426][SQL] Handles subexpression elimination con...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22417
**[Test build #96058 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96058/testReport)** for PR 22417 at commit [`35e7911`](https://github.com/apache/spark/commit/35e7911958d5be8d7b66803ba7e11cd22bc4bbe5).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22417: [SPARK-25426][SQL] Handles subexpression elimination con...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22417
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 #22417: [SPARK-25426][SQL] Remove the duplicate fallback logic i...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22417
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96074/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22417: [SPARK-25426][SQL] Remove the duplicate fallback logic i...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/22417
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22417: [SPARK-25426][SQL] Handles subexpression elimination con...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22417
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96073/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22417: [SPARK-25426][SQL] Remove the duplicate fallback ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/22417
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22417: [SPARK-25426][SQL] Remove the duplicate fallback logic i...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on the issue:
https://github.com/apache/spark/pull/22417
cc: @cloud-fan
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22417: [SPARK-25426][SQL] Remove the duplicate fallback logic i...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22417
**[Test build #96074 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96074/testReport)** for PR 22417 at commit [`e59902a`](https://github.com/apache/spark/commit/e59902a15913d888325cb69fc22288d1c6ba5433).
* 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 #22417: [SPARK-25426][SQL] Handles subexpression elimination con...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22417
**[Test build #96073 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96073/testReport)** for PR 22417 at commit [`ea7e473`](https://github.com/apache/spark/commit/ea7e47350883d7fb18318e0040a5492dc4a4aa07).
* This patch **fails Scala style 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 #22417: [SPARK-25426][SQL] Remove the duplicate fallback logic i...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22417
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 #22417: [SPARK-25426][SQL] Remove the duplicate fallback logic i...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22417
**[Test build #96074 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96074/testReport)** for PR 22417 at commit [`e59902a`](https://github.com/apache/spark/commit/e59902a15913d888325cb69fc22288d1c6ba5433).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22417: [SPARK-25426][SQL] Handles subexpression elimination con...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22417
**[Test build #96058 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96058/testReport)** for PR 22417 at commit [`35e7911`](https://github.com/apache/spark/commit/35e7911958d5be8d7b66803ba7e11cd22bc4bbe5).
* 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 #22417: [SPARK-25426][SQL] Handles subexpression eliminat...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22417#discussion_r217619116
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala ---
@@ -59,6 +59,6 @@ abstract class CodeGeneratorWithInterpretedFallback[IN, OUT] extends Logging {
}
}
- protected def createCodeGeneratedObject(in: IN): OUT
+ protected def createCodeGeneratedObject(in: IN, subexpressionEliminationEnabled: Boolean): OUT
--- End diff --
If we are going to get config directly from `SQLConf`, why do we need to pass it as a parameter here?
Can we keep this API untouched and let `createCodeGeneratedObject` implementation to take care of it?
Like:
```scala
object UnsafeProjection
extends CodeGeneratorWithInterpretedFallback[Seq[Expression], UnsafeProjection] {
override protected def createCodeGeneratedObject(in: Seq[Expression]): UnsafeProjection = {
GenerateUnsafeProjection.generate(in, SQLConf.get.subexpressionEliminationEnabled)
}
...
}
```
I think it is much simpler.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22417: [SPARK-25426][SQL] Handles subexpression elimination con...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22417
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22417: [SPARK-25426][SQL] Remove the duplicate fallback logic i...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22417
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org