This is an automated email from the ASF dual-hosted git repository.
dongjoon pushed a commit to branch branch-2.4
in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/branch-2.4 by this push:
new 6ec74e2 [SPARK-34696][SQL][TESTS] Fix CodegenInterpretedPlanTest to
generate correct test cases
6ec74e2 is described below
commit 6ec74e2c9633bf602ce8f0a1e436616a18567ef7
Author: Dongjoon Hyun <[email protected]>
AuthorDate: Wed Mar 10 23:41:49 2021 -0800
[SPARK-34696][SQL][TESTS] Fix CodegenInterpretedPlanTest to generate
correct test cases
SPARK-23596 added `CodegenInterpretedPlanTest` at Apache Spark 2.4.0 in a
wrong way because `withSQLConf` depends on the execution time `SQLConf.get`
instead of `test` function declaration time. So, the following code executes
the test twice without controlling the `CodegenObjectFactoryMode`. This PR aims
to fix it correct and introduce a new function `testFallback`.
```scala
trait CodegenInterpretedPlanTest extends PlanTest {
override protected def test(
testName: String,
testTags: Tag*)(testFun: => Any)(implicit pos: source.Position):
Unit = {
val codegenMode = CodegenObjectFactoryMode.CODEGEN_ONLY.toString
val interpretedMode = CodegenObjectFactoryMode.NO_CODEGEN.toString
withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> codegenMode) {
super.test(testName + " (codegen path)", testTags: _*)(testFun)(pos)
}
withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> interpretedMode) {
super.test(testName + " (interpreted path)", testTags:
_*)(testFun)(pos)
}
}
}
```
1. We need to use like the following.
```scala
super.test(testName + " (codegen path)", testTags: _*)(
withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> codegenMode) { testFun
})(pos)
super.test(testName + " (interpreted path)", testTags: _*)(
withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> interpretedMode) {
testFun })(pos)
```
2. After we fix this behavior with the above code, several test cases
including SPARK-34596 and SPARK-34607 fail because they didn't work at both
`CODEGEN` and `INTERPRETED` mode. Those test cases only work at `FALLBACK`
mode. So, inevitably, we need to introduce `testFallback`.
No.
Pass the CIs.
Closes #31766 from dongjoon-hyun/SPARK-34596-SPARK-34607.
Lead-authored-by: Dongjoon Hyun <[email protected]>
Co-authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 5c4d8f95385ac97a66e5b491b5883ec770ae85bd)
Signed-off-by: Dongjoon Hyun <[email protected]>
---
.../catalyst/encoders/ExpressionEncoderSuite.scala | 37 ++++++++++++++--------
.../apache/spark/sql/catalyst/plans/PlanTest.scala | 18 +++++++----
2 files changed, 36 insertions(+), 19 deletions(-)
diff --git
a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoderSuite.scala
b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoderSuite.scala
index f0d61de..caeac49 100644
---
a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoderSuite.scala
+++
b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoderSuite.scala
@@ -158,15 +158,16 @@ class ExpressionEncoderSuite extends
CodegenInterpretedPlanTest with AnalysisTes
encodeDecodeTest(Seq(Seq("abc", "xyz"), Seq[String](null), null, Seq("1",
null, "2")),
"seq of seq of string")
- encodeDecodeTest(Array(31, -123, 4), "array of int")
+ encodeDecodeTest(Array(31, -123, 4), "array of int", useFallback = true)
encodeDecodeTest(Array("abc", "xyz"), "array of string")
encodeDecodeTest(Array("a", null, "x"), "array of string with null")
- encodeDecodeTest(Array.empty[Int], "empty array of int")
+ encodeDecodeTest(Array.empty[Int], "empty array of int", useFallback = true)
encodeDecodeTest(Array.empty[String], "empty array of string")
- encodeDecodeTest(Array(Array(31, -123), null, Array(4, 67)), "array of array
of int")
+ encodeDecodeTest(Array(Array(31, -123), null, Array(4, 67)), "array of array
of int",
+ useFallback = true)
encodeDecodeTest(Array(Array("abc", "xyz"), Array[String](null), null,
Array("1", null, "2")),
- "array of array of string")
+ "array of array of string", useFallback = true)
encodeDecodeTest(Map(1 -> "a", 2 -> "b"), "map")
encodeDecodeTest(Map(1 -> "a", 2 -> null), "map with null")
@@ -192,8 +193,9 @@ class ExpressionEncoderSuite extends
CodegenInterpretedPlanTest with AnalysisTes
encoderFor(Encoders.javaSerialization[JavaSerializable]))
// test product encoders
- private def productTest[T <: Product : ExpressionEncoder](input: T): Unit = {
- encodeDecodeTest(input, input.getClass.getSimpleName)
+ private def productTest[T <: Product : ExpressionEncoder](
+ input: T, useFallback: Boolean = false): Unit = {
+ encodeDecodeTest(input, input.getClass.getSimpleName, useFallback)
}
case class InnerClass(i: Int)
@@ -211,7 +213,8 @@ class ExpressionEncoderSuite extends
CodegenInterpretedPlanTest with AnalysisTes
productTest(OptionalData(None, None, None, None, None, None, None, None))
encodeDecodeTest(Seq(Some(1), None), "Option in array")
- encodeDecodeTest(Map(1 -> Some(10L), 2 -> Some(20L), 3 -> None), "Option in
map")
+ encodeDecodeTest(Map(1 -> Some(10L), 2 -> Some(20L), 3 -> None), "Option in
map",
+ useFallback = true)
productTest(BoxedData(1, 1L, 1.0, 1.0f, 1.toShort, 1.toByte, true))
@@ -229,7 +232,7 @@ class ExpressionEncoderSuite extends
CodegenInterpretedPlanTest with AnalysisTes
Map(1 -> null),
PrimitiveData(1, 1, 1, 1, 1, 1, true)))
- productTest(NestedArray(Array(Array(1, -2, 3), null, Array(4, 5, -6))))
+ productTest(NestedArray(Array(Array(1, -2, 3), null, Array(4, 5, -6))),
useFallback = true)
productTest(("Seq[(String, String)]",
Seq(("a", "b"))))
@@ -372,8 +375,9 @@ class ExpressionEncoderSuite extends
CodegenInterpretedPlanTest with AnalysisTes
private def encodeDecodeTest[T : ExpressionEncoder](
input: T,
- testName: String): Unit = {
- testAndVerifyNotLeakingReflectionObjects(s"encode/decode for $testName:
$input") {
+ testName: String,
+ useFallback: Boolean = false): Unit = {
+ testAndVerifyNotLeakingReflectionObjects(s"encode/decode for $testName:
$input", useFallback) {
val encoder = implicitly[ExpressionEncoder[T]]
// Make sure encoder is serializable.
@@ -467,9 +471,16 @@ class ExpressionEncoderSuite extends
CodegenInterpretedPlanTest with AnalysisTes
r
}
- private def testAndVerifyNotLeakingReflectionObjects(testName:
String)(testFun: => Any) {
- test(testName) {
- verifyNotLeakingReflectionObjects(testFun)
+ private def testAndVerifyNotLeakingReflectionObjects(
+ testName: String, useFallback: Boolean = false)(testFun: => Any): Unit =
{
+ if (useFallback) {
+ testFallback(testName) {
+ verifyNotLeakingReflectionObjects(testFun)
+ }
+ } else {
+ test(testName) {
+ verifyNotLeakingReflectionObjects(testFun)
+ }
}
}
}
diff --git
a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/PlanTest.scala
b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/PlanTest.scala
index 67740c3..44491d9 100644
---
a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/PlanTest.scala
+++
b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/PlanTest.scala
@@ -44,12 +44,18 @@ trait CodegenInterpretedPlanTest extends PlanTest {
val codegenMode = CodegenObjectFactoryMode.CODEGEN_ONLY.toString
val interpretedMode = CodegenObjectFactoryMode.NO_CODEGEN.toString
- withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> codegenMode) {
- super.test(testName + " (codegen path)", testTags: _*)(testFun)(pos)
- }
- withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> interpretedMode) {
- super.test(testName + " (interpreted path)", testTags: _*)(testFun)(pos)
- }
+ super.test(testName + " (codegen path)", testTags: _*)(
+ withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> codegenMode) { testFun
})(pos)
+ super.test(testName + " (interpreted path)", testTags: _*)(
+ withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> interpretedMode) {
testFun })(pos)
+ }
+
+ protected def testFallback(
+ testName: String,
+ testTags: Tag*)(testFun: => Any)(implicit pos: source.Position): Unit = {
+ val codegenMode = CodegenObjectFactoryMode.FALLBACK.toString
+ super.test(testName + " (codegen fallback mode)", testTags: _*)(
+ withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> codegenMode) { testFun
})(pos)
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
