spark git commit: [SPARK-22591][SQL] GenerateOrdering shouldn't change CodegenContext.INPUT_ROW

2017-11-24 Thread wenchen 
Repository: spark
Updated Branches:
  refs/heads/branch-2.2 f8e73d029 -> f4c457a30


[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.

Author: Liang-Chi Hsieh 

Closes #19800 from viirya/SPARK-22591.

(cherry picked from commit 62a826f17c549ed93300bdce562db56bddd5d959)
Signed-off-by: Wenchen Fan 


Project: http://git-wip-us.apache.org/repos/asf/spark/repo
Commit: http://git-wip-us.apache.org/repos/asf/spark/commit/f4c457a3
Tree: http://git-wip-us.apache.org/repos/asf/spark/tree/f4c457a3
Diff: http://git-wip-us.apache.org/repos/asf/spark/diff/f4c457a3

Branch: refs/heads/branch-2.2
Commit: f4c457a308b0226aa0e7a1714c19046376e03409
Parents: f8e73d0
Author: Liang-Chi Hsieh 
Authored: Fri Nov 24 11:46:58 2017 +0100
Committer: Wenchen Fan 
Committed: Fri Nov 24 11:47:10 2017 +0100

--
 .../expressions/codegen/GenerateOrdering.scala  | 16 ++--
 .../sql/catalyst/expressions/OrderingSuite.scala| 11 ++-
 2 files changed, 20 insertions(+), 7 deletions(-)
--


http://git-wip-us.apache.org/repos/asf/spark/blob/f4c457a3/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala
--
diff --git 
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala
 
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala
index f7fc2d5..a2fe55b 100644
--- 
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala
+++ 
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala
@@ -72,13 +72,15 @@ 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 oldCurrentVars = ctx.currentVars
+val inputRow = "i"
+ctx.INPUT_ROW = inputRow
+// to use INPUT_ROW we must make sure currentVars is null
+ctx.currentVars = null
+
 val comparisons = ordering.map { order =>
-  val oldCurrentVars = ctx.currentVars
-  ctx.INPUT_ROW = "i"
-  // to use INPUT_ROW we must make sure currentVars is null
-  ctx.currentVars = null
   val eval = order.child.genCode(ctx)
-  ctx.currentVars = oldCurrentVars
   val asc = order.isAscending
   val isNullA = ctx.freshName("isNullA")
   val primitiveA = ctx.freshName("primitiveA")
@@ -147,10 +149,12 @@ object GenerateOrdering extends 
CodeGenerator[Seq[SortOrder], Ordering[InternalR
   """
 }.mkString
   })
+ctx.currentVars = oldCurrentVars
+ctx.INPUT_ROW = oldInputRow
 // make sure INPUT_ROW is declared even if splitExpressions
 // returns an inlined block
 s"""
-   |InternalRow ${ctx.INPUT_ROW} = null;
+   |InternalRow $inputRow = null;
|$code
  """.stripMargin
   }

http://git-wip-us.apache.org/repos/asf/spark/blob/f4c457a3/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/OrderingSuite.scala
--
diff --git 
a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/OrderingSuite.scala
 
b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/OrderingSuite.scala
index aa61ba2..d0604b8 100644
--- 
a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/OrderingSuite.scala
+++ 
b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/OrderingSuite.scala
@@ -24,7 +24,7 @@ import org.apache.spark.serializer.KryoSerializer
 import org.apache.spark.sql.{RandomDataGenerator, Row}
 import org.apache.spark.sql.catalyst.{CatalystTypeConverters, InternalRow}
 import org.apache.spark.sql.catalyst.dsl.expressions._
-import org.apache.spark.sql.catalyst.expressions.codegen.{GenerateOrdering, 
LazilyGeneratedOrdering}
+import org.apache.spark.sql.catalyst.expressions.codegen.{CodegenContext, 
GenerateOrdering, LazilyGeneratedOrdering}
 import org.apache.spark.sql.types._
 
 class OrderingSuite extends SparkFunSuite with ExpressionEvalHelper {
@@ -156,4 +156,13 @@ class OrderingSuite extends

spark git commit: [SPARK-22591][SQL] GenerateOrdering shouldn't change CodegenContext.INPUT_ROW

2017-11-24 Thread wenchen 
Repository: spark
Updated Branches:
  refs/heads/master c1217565e -> 62a826f17


[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.

Author: Liang-Chi Hsieh 

Closes #19800 from viirya/SPARK-22591.


Project: http://git-wip-us.apache.org/repos/asf/spark/repo
Commit: http://git-wip-us.apache.org/repos/asf/spark/commit/62a826f1
Tree: http://git-wip-us.apache.org/repos/asf/spark/tree/62a826f1
Diff: http://git-wip-us.apache.org/repos/asf/spark/diff/62a826f1

Branch: refs/heads/master
Commit: 62a826f17c549ed93300bdce562db56bddd5d959
Parents: c121756
Author: Liang-Chi Hsieh 
Authored: Fri Nov 24 11:46:58 2017 +0100
Committer: Wenchen Fan 
Committed: Fri Nov 24 11:46:58 2017 +0100

--
 .../expressions/codegen/GenerateOrdering.scala  | 16 ++--
 .../sql/catalyst/expressions/OrderingSuite.scala| 11 ++-
 2 files changed, 20 insertions(+), 7 deletions(-)
--


http://git-wip-us.apache.org/repos/asf/spark/blob/62a826f1/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala
--
diff --git 
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala
 
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala
index 1639d1b..4a45957 100644
--- 
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala
+++ 
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala
@@ -72,13 +72,15 @@ 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 oldCurrentVars = ctx.currentVars
+val inputRow = "i"
+ctx.INPUT_ROW = inputRow
+// to use INPUT_ROW we must make sure currentVars is null
+ctx.currentVars = null
+
 val comparisons = ordering.map { order =>
-  val oldCurrentVars = ctx.currentVars
-  ctx.INPUT_ROW = "i"
-  // to use INPUT_ROW we must make sure currentVars is null
-  ctx.currentVars = null
   val eval = order.child.genCode(ctx)
-  ctx.currentVars = oldCurrentVars
   val asc = order.isAscending
   val isNullA = ctx.freshName("isNullA")
   val primitiveA = ctx.freshName("primitiveA")
@@ -147,10 +149,12 @@ object GenerateOrdering extends 
CodeGenerator[Seq[SortOrder], Ordering[InternalR
   """
 }.mkString
   })
+ctx.currentVars = oldCurrentVars
+ctx.INPUT_ROW = oldInputRow
 // make sure INPUT_ROW is declared even if splitExpressions
 // returns an inlined block
 s"""
-   |InternalRow ${ctx.INPUT_ROW} = null;
+   |InternalRow $inputRow = null;
|$code
  """.stripMargin
   }

http://git-wip-us.apache.org/repos/asf/spark/blob/62a826f1/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/OrderingSuite.scala
--
diff --git 
a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/OrderingSuite.scala
 
b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/OrderingSuite.scala
index aa61ba2..d0604b8 100644
--- 
a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/OrderingSuite.scala
+++ 
b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/OrderingSuite.scala
@@ -24,7 +24,7 @@ import org.apache.spark.serializer.KryoSerializer
 import org.apache.spark.sql.{RandomDataGenerator, Row}
 import org.apache.spark.sql.catalyst.{CatalystTypeConverters, InternalRow}
 import org.apache.spark.sql.catalyst.dsl.expressions._
-import org.apache.spark.sql.catalyst.expressions.codegen.{GenerateOrdering, 
LazilyGeneratedOrdering}
+import org.apache.spark.sql.catalyst.expressions.codegen.{CodegenContext, 
GenerateOrdering, LazilyGeneratedOrdering}
 import org.apache.spark.sql.types._
 
 class OrderingSuite extends SparkFunSuite with ExpressionEvalHelper {
@@ -156,4 +156,13 @@ class OrderingSuite extends SparkFunSuite with 
ExpressionEvalHelper {
   assert(genOrdering.compare(rowB1, rowB2) < 0)
 }
   }
+
+  test("SPARK-22591: