From 121d309eab44f4a64b059e497c2c138cd4fb3695 Mon Sep 17 00:00:00 2001 From: Liang-Chi Hsieh Date: Thu, 23 Nov 2017 12:12:34 +0000 Subject: [PATCH 1/3] GenerateOrdering shouldn't change ctx.INPUT_ROW. --- .../expressions/codegen/GenerateOrdering.scala | 5 ++++- .../sql/catalyst/expressions/OrderingSuite.scala | 11 ++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) 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 1639d1b9dda1..3ebcf799c476 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,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 ctx.INPUT_ROW = "i" @@ -149,10 +150,12 @@ 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 + ctx.INPUT_ROW = oldInputRow + finalCode } protected def create(ordering: Seq[SortOrder]): BaseOrdering = { 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 aa61ba2bff2b..d0604b8eb767 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: GenerateOrdering shouldn't change ctx.INPUT_ROW") { + val ctx = new CodegenContext() + ctx.INPUT_ROW = null + + val schema = new StructType().add("field", FloatType, nullable = true) + GenerateOrdering.genComparisons(ctx, schema) + assert(ctx.INPUT_ROW == null) + } } From 27145ae94139eb7e3c221f7f4fa598d84cccbb2d Mon Sep 17 00:00:00 2001 From: Liang-Chi Hsieh Date: Thu, 23 Nov 2017 13:29:04 +0000 Subject: [PATCH 2/3] Address comment. --- .../expressions/codegen/GenerateOrdering.scala | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) 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 3ebcf799c476..4ad6d128129a 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 @@ -73,13 +73,14 @@ object GenerateOrdering extends CodeGenerator[Seq[SortOrder], Ordering[InternalR */ 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") @@ -154,6 +155,8 @@ object GenerateOrdering extends CodeGenerator[Seq[SortOrder], Ordering[InternalR |InternalRow ${ctx.INPUT_ROW} = null; |$code """.stripMargin + // Restore original currentVars and INPUT_ROW. + ctx.currentVars = oldCurrentVars ctx.INPUT_ROW = oldInputRow finalCode } From 7462a55a4256143c39fdaacfdaf334dfc4ba8b7d Mon Sep 17 00:00:00 2001 From: Liang-Chi Hsieh Date: Fri, 24 Nov 2017 00:00:13 +0000 Subject: [PATCH 3/3] Address comment. --- .../expressions/codegen/GenerateOrdering.scala | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) 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 4ad6d128129a..4a459571ed63 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 @@ -149,16 +149,14 @@ 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 - val finalCode = s""" - |InternalRow ${ctx.INPUT_ROW} = null; + s""" + |InternalRow $inputRow = null; |$code """.stripMargin - // Restore original currentVars and INPUT_ROW. - ctx.currentVars = oldCurrentVars - ctx.INPUT_ROW = oldInputRow - finalCode } protected def create(ordering: Seq[SortOrder]): BaseOrdering = {