spark git commit: [SPARK-16489][SQL] Guard against variable reuse mistakes in expression code generation

[rxin]

Repository: spark
Updated Branches:
  refs/heads/branch-1.6 d1c992fea -> 7c8a399a2


[SPARK-16489][SQL] Guard against variable reuse mistakes in expression code 
generation

In code generation, it is incorrect for expressions to reuse variable names 
across different instances of itself. As an example, SPARK-16488 reports a bug 
in which pmod expression reuses variable name "r".

This patch updates ExpressionEvalHelper test harness to always project two 
instances of the same expression, which will help us catch variable reuse 
problems in expression unit tests. This patch also fixes the bug in crc32 
expression.

This is a test harness change, but I also created a new test suite for testing 
the test harness.

Author: Reynold Xin 

Closes #14146 from rxin/SPARK-16489.

(cherry picked from commit c377e49e38a290e5c4fbc178278069788674dfb7)
Signed-off-by: Reynold Xin 


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

Branch: refs/heads/branch-1.6
Commit: 7c8a399a292de113ebec4235ebe21c9a8fb85256
Parents: d1c992f
Author: Reynold Xin 
Authored: Tue Jul 12 10:07:23 2016 -0700
Committer: Reynold Xin 
Committed: Tue Jul 12 23:14:17 2016 -0700

--
 .../spark/sql/catalyst/expressions/misc.scala   |  7 +--
 .../expressions/ExpressionEvalHelper.scala  | 15 --
 .../expressions/ExpressionEvalHelperSuite.scala | 54 
 3 files changed, 68 insertions(+), 8 deletions(-)
--


http://git-wip-us.apache.org/repos/asf/spark/blob/7c8a399a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
--
diff --git 
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
 
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
index 0f6d02f..cf9403c 100644
--- 
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
+++ 
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
@@ -151,11 +151,12 @@ case class Crc32(child: Expression) extends 
UnaryExpression with ImplicitCastInp
 
   override def genCode(ctx: CodeGenContext, ev: GeneratedExpressionCode): 
String = {
 val CRC32 = "java.util.zip.CRC32"
+val checksum = ctx.freshName("checksum")
 nullSafeCodeGen(ctx, ev, value => {
   s"""
-$CRC32 checksum = new $CRC32();
-checksum.update($value, 0, $value.length);
-${ev.value} = checksum.getValue();
+$CRC32 $checksum = new $CRC32();
+$checksum.update($value, 0, $value.length);
+${ev.value} = $checksum.getValue();
   """
 })
   }

http://git-wip-us.apache.org/repos/asf/spark/blob/7c8a399a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
--
diff --git 
a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
 
b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
index 074785e..9f463c5 100644
--- 
a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
+++ 
b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
@@ -160,9 +160,13 @@ trait ExpressionEvalHelper extends 
GeneratorDrivenPropertyChecks {
   expression: Expression,
   expected: Any,
   inputRow: InternalRow = EmptyRow): Unit = {
-
+// SPARK-16489 Explicitly doing code generation twice so code gen will 
fail if
+// some expression is reusing variable names across different instances.
+// This behavior is tested in ExpressionEvalHelperSuite.
 val plan = generateProject(
-  GenerateUnsafeProjection.generate(Alias(expression, 
s"Optimized($expression)")() :: Nil),
+  GenerateUnsafeProjection.generate(
+Alias(expression, s"Optimized($expression)1")() ::
+  Alias(expression, s"Optimized($expression)2")() :: Nil),
   expression)
 
 val unsafeRow = plan(inputRow)
@@ -170,13 +174,14 @@ trait ExpressionEvalHelper extends 
GeneratorDrivenPropertyChecks {
 
 if (expected == null) {
   if (!unsafeRow.isNullAt(0)) {
-val expectedRow = InternalRow(expected)
+val expectedRow = InternalRow(expected, expected)
 fail("Incorrect evaluation in unsafe mode: " +
   s"$expression, actual: $unsafeRow, expected: $expectedRow$input")
   }
 } else {
-  val lit = InternalRow(expected)
-  val 

spark git commit: [SPARK-16489][SQL] Guard against variable reuse mistakes in expression code generation

[rxin]

Repository: spark
Updated Branches:
  refs/heads/branch-2.0 7b63e7d92 -> f41947654


[SPARK-16489][SQL] Guard against variable reuse mistakes in expression code 
generation

In code generation, it is incorrect for expressions to reuse variable names 
across different instances of itself. As an example, SPARK-16488 reports a bug 
in which pmod expression reuses variable name "r".

This patch updates ExpressionEvalHelper test harness to always project two 
instances of the same expression, which will help us catch variable reuse 
problems in expression unit tests. This patch also fixes the bug in crc32 
expression.

This is a test harness change, but I also created a new test suite for testing 
the test harness.

Author: Reynold Xin 

Closes #14146 from rxin/SPARK-16489.

(cherry picked from commit c377e49e38a290e5c4fbc178278069788674dfb7)
Signed-off-by: Reynold Xin 


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

Branch: refs/heads/branch-2.0
Commit: f419476546f133040a21d7662b6509185f1a5d53
Parents: 7b63e7d
Author: Reynold Xin 
Authored: Tue Jul 12 10:07:23 2016 -0700
Committer: Reynold Xin 
Committed: Tue Jul 12 10:08:07 2016 -0700

--
 .../spark/sql/catalyst/expressions/misc.scala   |  7 +--
 .../expressions/ExpressionEvalHelper.scala  | 15 --
 .../expressions/ExpressionEvalHelperSuite.scala | 54 
 .../sql/test/DataFrameReaderWriterSuite.scala   | 14 -
 4 files changed, 68 insertions(+), 22 deletions(-)
--


http://git-wip-us.apache.org/repos/asf/spark/blob/f4194765/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
--
diff --git 
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
 
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
index 1c0787b..d2c94ec 100644
--- 
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
+++ 
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
@@ -175,11 +175,12 @@ case class Crc32(child: Expression) extends 
UnaryExpression with ImplicitCastInp
 
   override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
 val CRC32 = "java.util.zip.CRC32"
+val checksum = ctx.freshName("checksum")
 nullSafeCodeGen(ctx, ev, value => {
   s"""
-$CRC32 checksum = new $CRC32();
-checksum.update($value, 0, $value.length);
-${ev.value} = checksum.getValue();
+$CRC32 $checksum = new $CRC32();
+$checksum.update($value, 0, $value.length);
+${ev.value} = $checksum.getValue();
   """
 })
   }

http://git-wip-us.apache.org/repos/asf/spark/blob/f4194765/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
--
diff --git 
a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
 
b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
index 58e9d6f..d6a9672 100644
--- 
a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
+++ 
b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
@@ -132,9 +132,13 @@ trait ExpressionEvalHelper extends 
GeneratorDrivenPropertyChecks {
   expression: Expression,
   expected: Any,
   inputRow: InternalRow = EmptyRow): Unit = {
-
+// SPARK-16489 Explicitly doing code generation twice so code gen will 
fail if
+// some expression is reusing variable names across different instances.
+// This behavior is tested in ExpressionEvalHelperSuite.
 val plan = generateProject(
-  GenerateUnsafeProjection.generate(Alias(expression, 
s"Optimized($expression)")() :: Nil),
+  GenerateUnsafeProjection.generate(
+Alias(expression, s"Optimized($expression)1")() ::
+  Alias(expression, s"Optimized($expression)2")() :: Nil),
   expression)
 
 val unsafeRow = plan(inputRow)
@@ -142,13 +146,14 @@ trait ExpressionEvalHelper extends 
GeneratorDrivenPropertyChecks {
 
 if (expected == null) {
   if (!unsafeRow.isNullAt(0)) {
-val expectedRow = InternalRow(expected)
+val expectedRow = InternalRow(expected, expected)
 fail("Incorrect evaluation in unsafe mode: " +
   s"$expression, actual: $unsafeRow, expected: $expectedRow$input")
   }
 } else {
- 

spark git commit: [SPARK-16489][SQL] Guard against variable reuse mistakes in expression code generation

[rxin]

Repository: spark
Updated Branches:
  refs/heads/master 5ad68ba5c -> c377e49e3


[SPARK-16489][SQL] Guard against variable reuse mistakes in expression code 
generation

## What changes were proposed in this pull request?
In code generation, it is incorrect for expressions to reuse variable names 
across different instances of itself. As an example, SPARK-16488 reports a bug 
in which pmod expression reuses variable name "r".

This patch updates ExpressionEvalHelper test harness to always project two 
instances of the same expression, which will help us catch variable reuse 
problems in expression unit tests. This patch also fixes the bug in crc32 
expression.

## How was this patch tested?
This is a test harness change, but I also created a new test suite for testing 
the test harness.

Author: Reynold Xin 

Closes #14146 from rxin/SPARK-16489.


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

Branch: refs/heads/master
Commit: c377e49e38a290e5c4fbc178278069788674dfb7
Parents: 5ad68ba
Author: Reynold Xin 
Authored: Tue Jul 12 10:07:23 2016 -0700
Committer: Reynold Xin 
Committed: Tue Jul 12 10:07:23 2016 -0700

--
 .../spark/sql/catalyst/expressions/misc.scala   |  7 +--
 .../expressions/ExpressionEvalHelper.scala  | 15 --
 .../expressions/ExpressionEvalHelperSuite.scala | 54 
 .../sql/test/DataFrameReaderWriterSuite.scala   | 14 -
 4 files changed, 68 insertions(+), 22 deletions(-)
--


http://git-wip-us.apache.org/repos/asf/spark/blob/c377e49e/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
--
diff --git 
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
 
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
index 1c0787b..d2c94ec 100644
--- 
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
+++ 
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
@@ -175,11 +175,12 @@ case class Crc32(child: Expression) extends 
UnaryExpression with ImplicitCastInp
 
   override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
 val CRC32 = "java.util.zip.CRC32"
+val checksum = ctx.freshName("checksum")
 nullSafeCodeGen(ctx, ev, value => {
   s"""
-$CRC32 checksum = new $CRC32();
-checksum.update($value, 0, $value.length);
-${ev.value} = checksum.getValue();
+$CRC32 $checksum = new $CRC32();
+$checksum.update($value, 0, $value.length);
+${ev.value} = $checksum.getValue();
   """
 })
   }

http://git-wip-us.apache.org/repos/asf/spark/blob/c377e49e/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
--
diff --git 
a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
 
b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
index 58e9d6f..d6a9672 100644
--- 
a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
+++ 
b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
@@ -132,9 +132,13 @@ trait ExpressionEvalHelper extends 
GeneratorDrivenPropertyChecks {
   expression: Expression,
   expected: Any,
   inputRow: InternalRow = EmptyRow): Unit = {
-
+// SPARK-16489 Explicitly doing code generation twice so code gen will 
fail if
+// some expression is reusing variable names across different instances.
+// This behavior is tested in ExpressionEvalHelperSuite.
 val plan = generateProject(
-  GenerateUnsafeProjection.generate(Alias(expression, 
s"Optimized($expression)")() :: Nil),
+  GenerateUnsafeProjection.generate(
+Alias(expression, s"Optimized($expression)1")() ::
+  Alias(expression, s"Optimized($expression)2")() :: Nil),
   expression)
 
 val unsafeRow = plan(inputRow)
@@ -142,13 +146,14 @@ trait ExpressionEvalHelper extends 
GeneratorDrivenPropertyChecks {
 
 if (expected == null) {
   if (!unsafeRow.isNullAt(0)) {
-val expectedRow = InternalRow(expected)
+val expectedRow = InternalRow(expected, expected)
 fail("Incorrect evaluation in unsafe mode: " +
   s"$expression, actual: $unsafeRow, expected: $expectedRow$input")
   }
 } else {
-  val lit = InternalRow(expected)
-  val