You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by viirya <gi...@git.apache.org> on 2017/11/23 12:14:10 UTC
[GitHub] spark pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...
GitHub user viirya opened a pull request:
https://github.com/apache/spark/pull/19800
[SPARK-22591][SQL] GenerateOrdering shouldn't change CodegenContext.INPUT_ROW
## What changes were proposed in this pull request?
When I played with codegen in developing another PR, I found the value of `CodegenContext.INPUT_ROW` is not reliable. Under wholestage codegen, it is assigned to null first and then suddenly changed to `i`.
The reason is `GenerateOrdering` changes `CodegenContext.INPUT_ROW` but doesn't restore it back.
## How was this patch tested?
Added test.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/viirya/spark-1 SPARK-22591
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19800.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 #19800
----
commit 121d309eab44f4a64b059e497c2c138cd4fb3695
Author: Liang-Chi Hsieh <vi...@gmail.com>
Date: 2017-11-23T12:12:34Z
GenerateOrdering shouldn't change ctx.INPUT_ROW.
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19800#discussion_r152799244
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala ---
@@ -72,6 +72,7 @@ object GenerateOrdering extends CodeGenerator[Seq[SortOrder], Ordering[InternalR
* Generates the code for ordering based on the given order.
*/
def genComparisons(ctx: CodegenContext, ordering: Seq[SortOrder]): String = {
+ val oldInputRow = ctx.INPUT_ROW
val comparisons = ordering.map { order =>
val oldCurrentVars = ctx.currentVars
--- End diff --
how about
```
val oldInputRow = ...
val oldCurrentVars = ...
val inputRow = "i"
ctx.INPUT_ROW = inputRow
ctx.currentVars = null
val comparisons = ...
ctx.INPUT_ROW = oldInputRow
ctx.currentVars = oldCurrentVars
...
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't change Cod...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19800
**[Test build #84128 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84128/testReport)** for PR 19800 at commit [`121d309`](https://github.com/apache/spark/commit/121d309eab44f4a64b059e497c2c138cd4fb3695).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19800#discussion_r152794103
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/OrderingSuite.scala ---
@@ -156,4 +156,13 @@ class OrderingSuite extends SparkFunSuite with ExpressionEvalHelper {
assert(genOrdering.compare(rowB1, rowB2) < 0)
}
}
+
+ test("SPARK-22591: GenerateOrdering shouldn't change ctx.INPUT_ROW") {
--- End diff --
I'm working to support wholestage codegen when generating expression codes safe from 64k limit. When there is not `INPUT_ROW` in context but we wrongly set a `INPUT_ROW` value, a non-existing `InternalRow i` will be added into function parameters.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19800#discussion_r152844926
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala ---
@@ -149,10 +151,14 @@ object GenerateOrdering extends CodeGenerator[Seq[SortOrder], Ordering[InternalR
})
// make sure INPUT_ROW is declared even if splitExpressions
// returns an inlined block
- s"""
+ val finalCode = s"""
|InternalRow ${ctx.INPUT_ROW} = null;
|$code
""".stripMargin
+ // Restore original currentVars and INPUT_ROW.
+ ctx.currentVars = oldCurrentVars
+ ctx.INPUT_ROW = oldInputRow
+ finalCode
--- End diff --
```
ctx.currentVars = oldCurrentVars
ctx.INPUT_ROW = oldInputRow
s"""
|InternalRow $inputRow = null;
|$code
""".stripMargin
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't change Cod...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19800
I created https://issues.apache.org/jira/browse/SPARK-22595 to track the flaky test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't change Cod...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19800
**[Test build #84128 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84128/testReport)** for PR 19800 at commit [`121d309`](https://github.com/apache/spark/commit/121d309eab44f4a64b059e497c2c138cd4fb3695).
* This patch **fails Spark unit 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 #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't change Cod...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19800
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84131/
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 #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19800#discussion_r152798521
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala ---
@@ -72,6 +72,7 @@ object GenerateOrdering extends CodeGenerator[Seq[SortOrder], Ordering[InternalR
* Generates the code for ordering based on the given order.
*/
def genComparisons(ctx: CodegenContext, ordering: Seq[SortOrder]): String = {
+ val oldInputRow = ctx.INPUT_ROW
val comparisons = ordering.map { order =>
val oldCurrentVars = ctx.currentVars
--- End diff --
seems we can also move this our of the loop.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't change Cod...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19800
**[Test build #84144 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84144/testReport)** for PR 19800 at commit [`7462a55`](https://github.com/apache/spark/commit/7462a55a4256143c39fdaacfdaf334dfc4ba8b7d).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't change Cod...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19800
**[Test build #84131 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84131/testReport)** for PR 19800 at commit [`27145ae`](https://github.com/apache/spark/commit/27145ae94139eb7e3c221f7f4fa598d84cccbb2d).
* 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 #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19800#discussion_r152791909
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/OrderingSuite.scala ---
@@ -156,4 +156,13 @@ class OrderingSuite extends SparkFunSuite with ExpressionEvalHelper {
assert(genOrdering.compare(rowB1, rowB2) < 0)
}
}
+
+ test("SPARK-22591: GenerateOrdering shouldn't change ctx.INPUT_ROW") {
--- End diff --
does it cause any bugs?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't change Cod...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19800
thanks, merging to master/2.2!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19800#discussion_r152799599
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala ---
@@ -72,6 +72,7 @@ object GenerateOrdering extends CodeGenerator[Seq[SortOrder], Ordering[InternalR
* Generates the code for ordering based on the given order.
*/
def genComparisons(ctx: CodegenContext, ordering: Seq[SortOrder]): String = {
+ val oldInputRow = ctx.INPUT_ROW
val comparisons = ordering.map { order =>
val oldCurrentVars = ctx.currentVars
--- End diff --
Ok. Looks good.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't change Cod...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19800
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84144/
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 #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19800#discussion_r152798856
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala ---
@@ -72,6 +72,7 @@ object GenerateOrdering extends CodeGenerator[Seq[SortOrder], Ordering[InternalR
* Generates the code for ordering based on the given order.
*/
def genComparisons(ctx: CodegenContext, ordering: Seq[SortOrder]): String = {
+ val oldInputRow = ctx.INPUT_ROW
val comparisons = ordering.map { order =>
val oldCurrentVars = ctx.currentVars
--- End diff --
Yes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't change Cod...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19800
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 #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't change Cod...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19800
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 pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19800#discussion_r152798443
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/OrderingSuite.scala ---
@@ -156,4 +156,13 @@ class OrderingSuite extends SparkFunSuite with ExpressionEvalHelper {
assert(genOrdering.compare(rowB1, rowB2) < 0)
}
}
+
+ test("SPARK-22591: GenerateOrdering shouldn't change ctx.INPUT_ROW") {
--- End diff --
As we use `INPUT_ROW` a lot in codegen framework, it is risky to change its value without restoring it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't change Cod...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19800
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 #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't change Cod...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19800
This test `org.apache.spark.sql.catalyst.expressions.CastSuite.SPARK-22500: cast for struct should not generate codes beyond 64KB` seems flaky? I saw it fails with `java.lang.OutOfMemoryError: GC overhead limit exceeded` in other PR too.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/19800
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't change Cod...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19800
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84128/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't change Cod...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19800
**[Test build #84144 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84144/testReport)** for PR 19800 at commit [`7462a55`](https://github.com/apache/spark/commit/7462a55a4256143c39fdaacfdaf334dfc4ba8b7d).
* 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 #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't change Cod...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19800
cc @cloud-fan @kiszk
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19800#discussion_r152881639
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala ---
@@ -149,10 +151,14 @@ object GenerateOrdering extends CodeGenerator[Seq[SortOrder], Ordering[InternalR
})
// make sure INPUT_ROW is declared even if splitExpressions
// returns an inlined block
- s"""
+ val finalCode = s"""
|InternalRow ${ctx.INPUT_ROW} = null;
|$code
""".stripMargin
+ // Restore original currentVars and INPUT_ROW.
+ ctx.currentVars = oldCurrentVars
+ ctx.INPUT_ROW = oldInputRow
+ finalCode
--- End diff --
Yes, thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't change Cod...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19800
**[Test build #84131 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84131/testReport)** for PR 19800 at commit [`27145ae`](https://github.com/apache/spark/commit/27145ae94139eb7e3c221f7f4fa598d84cccbb2d).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org