[GitHub] spark pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/19800


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...

[viirya]

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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...

[cloud-fan]

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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...

[viirya]

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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...

[cloud-fan]

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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...

[viirya]

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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...

[cloud-fan]

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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...

[viirya]

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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...

[viirya]

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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...

[cloud-fan]

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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19800: [SPARK-22591][SQL] GenerateOrdering shouldn't cha...

[viirya]

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 
Date:   2017-11-23T12:12:34Z

GenerateOrdering shouldn't change ctx.INPUT_ROW.




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org