This is an automated email from the ASF dual-hosted git repository. dongjoon pushed a commit to branch branch-3.1 in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/branch-3.1 by this push: new 2f88e77 [SPARK-34696][SQL][TESTS] Fix CodegenInterpretedPlanTest to generate correct test cases 2f88e77 is described below commit 2f88e77031607d1a5d0cdda93ccc706ad45c78b5 Author: Dongjoon Hyun <dh...@apple.com> 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 <dh...@apple.com> Co-authored-by: Dongjoon Hyun <dongj...@apache.org> Signed-off-by: Dongjoon Hyun <dh...@apple.com> (cherry picked from commit 5c4d8f95385ac97a66e5b491b5883ec770ae85bd) Signed-off-by: Dongjoon Hyun <dh...@apple.com> --- .../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: commits-unsubscr...@spark.apache.org For additional commands, e-mail: commits-h...@spark.apache.org