This is an automated email from the ASF dual-hosted git repository.
dongjoon pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/master by this push:
new 5c4d8f9 [SPARK-34696][SQL][TESTS] Fix CodegenInterpretedPlanTest to
generate correct test cases
5c4d8f9 is described below
commit 5c4d8f95385ac97a66e5b491b5883ec770ae85bd
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
### What changes were proposed in this pull request?
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)
}
}
}
```
### Why are the changes needed?
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`.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
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]>
---
.../catalyst/encoders/ExpressionEncoderSuite.scala | 49 ++++++++++++++--------
.../apache/spark/sql/catalyst/plans/PlanTest.scala | 18 +++++---
2 files changed, 44 insertions(+), 23 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 095f6a9..6c2da4d3 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
@@ -161,15 +161,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")
@@ -195,8 +196,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)
@@ -214,7 +216,8 @@ class ExpressionEncoderSuite extends
CodegenInterpretedPlanTest with AnalysisTes
OuterScopes.addOuterScope(MalformedClassObject)
encodeDecodeTest(
MalformedClassObject.MalformedNameExample(42),
- "nested Scala class should work")
+ "nested Scala class should work",
+ useFallback = true)
}
object OuterLevelWithVeryVeryVeryLongClassName1 {
@@ -284,7 +287,8 @@ class ExpressionEncoderSuite extends
CodegenInterpretedPlanTest with AnalysisTes
.OuterLevelWithVeryVeryVeryLongClassName19
.OuterLevelWithVeryVeryVeryLongClassName20
.MalformedNameExample(42),
- "deeply nested Scala class should work")
+ "deeply nested Scala class should work",
+ useFallback = true)
}
productTest(PrimitiveData(1, 1, 1, 1, 1, 1, true))
@@ -296,7 +300,8 @@ class ExpressionEncoderSuite extends
CodegenInterpretedPlanTest with AnalysisTes
productTest(OptionalData(None, 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))
@@ -314,7 +319,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"))))
@@ -474,8 +479,10 @@ class ExpressionEncoderSuite extends
CodegenInterpretedPlanTest with AnalysisTes
encodeDecodeTest((1, FooEnum.E1), "Tuple with Int and scala Enum")
encodeDecodeTest((null, FooEnum.E1, FooEnum.E2), "Tuple with Null and scala
Enum")
encodeDecodeTest(Seq(FooEnum.E1, null), "Seq with scala Enum")
- encodeDecodeTest(Map("key" -> FooEnum.E1), "Map with String key and scala
Enum")
- encodeDecodeTest(Map(FooEnum.E1 -> "value"), "Map with scala Enum key and
String value")
+ encodeDecodeTest(Map("key" -> FooEnum.E1), "Map with String key and scala
Enum",
+ useFallback = true)
+ encodeDecodeTest(Map(FooEnum.E1 -> "value"), "Map with scala Enum key and
String value",
+ useFallback = true)
encodeDecodeTest(FooClassWithEnum(1, FooEnum.E1), "case class with Int and
scala Enum")
encodeDecodeTest(FooEnum.E1, "scala Enum")
@@ -555,8 +562,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.
@@ -650,9 +658,16 @@ class ExpressionEncoderSuite extends
CodegenInterpretedPlanTest with AnalysisTes
r
}
- private def testAndVerifyNotLeakingReflectionObjects(testName:
String)(testFun: => Any): Unit = {
- 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 7c70ab9..3676f0c 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]
