Skip to content

[SPARK-34696][SQL][TESTS] Fix CodegenInterpretedPlanTest to generate correct test cases #31766

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dongjoon-hyun wants to merge 2 commits into from
Closed

[SPARK-34696][SQL][TESTS] Fix CodegenInterpretedPlanTest to generate correct test cases #31766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b2915d
[SPARK-34696][SQL][TESTS] Fix CodegenInterpretedPlanTest to generate …
dongjoon-hyun Mar 6, 2021
54636fc
use fallback for all failures
dongjoon-hyun Mar 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 32 additions & 17 deletions ...talyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoderSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

@cloud-fan cloud-fan Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we know why this simply test has to use fallback test?

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also was surprised that we have several cases instead of the new two tests.
It seems to be broken at some commits during the time because we didn't notice it due to this CodegenInterpretedPlanTest test framework bug.
Actually, I decided to focus on the test suite first and makes the recovery the beyond of the scope of this PR.

We may had better file a new JIRA aiming the recovery of those test cases one by one if possible.

BTW, for this specific instance, the following was the error.

[info] - encode/decode for array of int: [I@74655d03 (interpreted path) *** FAILED *** (14 milliseconds)
[info]   Exception thrown while decoding
[info]   Converted: [0,1000000020,3,0,ffffff850000001f,4]
[info]   Schema: value#680
[info]   root
[info]   -- value: array (nullable = true)
[info]       |-- element: integer (containsNull = false)
[info]   
[info]   
[info]   Encoder:
[info]   class[value[0]: array<int>] (ExpressionEncoderSuite.scala:578)
[info]   org.scalatest.exceptions.TestFailedException:
[info]   at org.scalatest.Assertions.newAssertionFailedException(Assertions.scala:472)
[info]   at org.scalatest.Assertions.newAssertionFailedException$(Assertions.scala:471)
[info]   at org.scalatest.funsuite.AnyFunSuite.newAssertionFailedException(AnyFunSuite.scala:1563)
[info]   at org.scalatest.Assertions.fail(Assertions.scala:949)
[info]   at org.scalatest.Assertions.fail$(Assertions.scala:945)
[info]   at org.scalatest.funsuite.AnyFunSuite.fail(AnyFunSuite.scala:1563)
[info]   at org.apache.spark.sql.catalyst.encoders.ExpressionEncoderSuite.$anonfun$encodeDecodeTest$1(ExpressionEncoderSuite.scala:578)
[info]   at org.apache.spark.sql.catalyst.encoders.ExpressionEncoderSuite.verifyNotLeakingReflectionObjects(ExpressionEncoderSuite.scala:656)
[info]   at org.apache.spark.sql.catalyst.encoders.ExpressionEncoderSuite.$anonfun$testAndVerifyNotLeakingReflectionObjects$2(ExpressionEncoderSuite.scala:669)
[info]   at org.apache.spark.sql.catalyst.plans.CodegenInterpretedPlanTest.$anonfun$test$4(PlanTest.scala:50)
[info]   at org.apache.spark.sql.catalyst.plans.SQLHelper.withSQLConf(SQLHelper.scala:54)
[info]   at org.apache.spark.sql.catalyst.plans.SQLHelper.withSQLConf$(SQLHelper.scala:38)
[info]   at org.apache.spark.sql.catalyst.encoders.ExpressionEncoderSuite.withSQLConf(ExpressionEncoderSuite.scala:118)
[info]   at org.apache.spark.sql.catalyst.plans.CodegenInterpretedPlanTest.$anonfun$test$3(PlanTest.scala:50)
[info]   at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23)
[info]   at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85)
[info]   at org.scalatest.OutcomeOf.outcomeOf$(OutcomeOf.scala:83)
[info]   at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104)
[info]   at org.scalatest.Transformer.apply(Transformer.scala:22)
[info]   at org.scalatest.Transformer.apply(Transformer.scala:20)
[info]   at org.scalatest.funsuite.AnyFunSuiteLike$$anon$1.apply(AnyFunSuiteLike.scala:190)
[info]   at org.apache.spark.SparkFunSuite.withFixture(SparkFunSuite.scala:178)
[info]   at org.scalatest.funsuite.AnyFunSuiteLike.invokeWithFixture$1(AnyFunSuiteLike.scala:188)
[info]   at org.scalatest.funsuite.AnyFunSuiteLike.$anonfun$runTest$1(AnyFunSuiteLike.scala:200)
[info]   at org.scalatest.SuperEngine.runTestImpl(Engine.scala:306)
[info]   at org.scalatest.funsuite.AnyFunSuiteLike.runTest(AnyFunSuiteLike.scala:200)
[info]   at org.scalatest.funsuite.AnyFunSuiteLike.runTest$(AnyFunSuiteLike.scala:182)
[info]   at org.apache.spark.SparkFunSuite.org$scalatest$BeforeAndAfterEach$$super$runTest(SparkFunSuite.scala:61)
[info]   at org.scalatest.BeforeAndAfterEach.runTest(BeforeAndAfterEach.scala:234)
[info]   at org.scalatest.BeforeAndAfterEach.runTest$(BeforeAndAfterEach.scala:227)
[info]   at org.apache.spark.SparkFunSuite.runTest(SparkFunSuite.scala:61)
[info]   at org.scalatest.funsuite.AnyFunSuiteLike.$anonfun$runTests$1(AnyFunSuiteLike.scala:233)
[info]   at org.scalatest.SuperEngine.$anonfun$runTestsInBranch$1(Engine.scala:413)
[info]   at scala.collection.immutable.List.foreach(List.scala:392)
[info]   at org.scalatest.SuperEngine.traverseSubNodes$1(Engine.scala:401)
[info]   at org.scalatest.SuperEngine.runTestsInBranch(Engine.scala:396)
[info]   at org.scalatest.SuperEngine.runTestsImpl(Engine.scala:475)
[info]   at org.scalatest.funsuite.AnyFunSuiteLike.runTests(AnyFunSuiteLike.scala:233)
[info]   at org.scalatest.funsuite.AnyFunSuiteLike.runTests$(AnyFunSuiteLike.scala:232)
[info]   at org.scalatest.funsuite.AnyFunSuite.runTests(AnyFunSuite.scala:1563)
[info]   at org.scalatest.Suite.run(Suite.scala:1112)
[info]   at org.scalatest.Suite.run$(Suite.scala:1094)
[info]   at org.scalatest.funsuite.AnyFunSuite.org$scalatest$funsuite$AnyFunSuiteLike$$super$run(AnyFunSuite.scala:1563)
[info]   at org.scalatest.funsuite.AnyFunSuiteLike.$anonfun$run$1(AnyFunSuiteLike.scala:237)
[info]   at org.scalatest.SuperEngine.runImpl(Engine.scala:535)
[info]   at org.scalatest.funsuite.AnyFunSuiteLike.run(AnyFunSuiteLike.scala:237)
[info]   at org.scalatest.funsuite.AnyFunSuiteLike.run$(AnyFunSuiteLike.scala:236)
[info]   at org.apache.spark.SparkFunSuite.org$scalatest$BeforeAndAfterAll$$super$run(SparkFunSuite.scala:61)
[info]   at org.scalatest.BeforeAndAfterAll.liftedTree1$1(BeforeAndAfterAll.scala:213)
[info]   at org.scalatest.BeforeAndAfterAll.run(BeforeAndAfterAll.scala:210)
[info]   at org.scalatest.BeforeAndAfterAll.run$(BeforeAndAfterAll.scala:208)
[info]   at org.apache.spark.SparkFunSuite.run(SparkFunSuite.scala:61)
[info]   at org.scalatest.tools.Framework.org$scalatest$tools$Framework$$runSuite(Framework.scala:318)
[info]   at org.scalatest.tools.Framework$ScalaTestTask.execute(Framework.scala:513)
[info]   at sbt.ForkMain$Run.lambda$runTest$1(ForkMain.java:413)
[info]   at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[info]   at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
[info]   at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
[info]   at java.lang.Thread.run(Thread.java:748)
[info]   Cause: java.lang.RuntimeException: Error while decoding: java.lang.NoSuchMethodException: org.apache.spark.sql.catalyst.util.GenericArrayData.toIntArray()
[info] mapobjects(lambdavariable(MapObject, IntegerType, false, -1), assertnotnull(lambdavariable(MapObject, IntegerType, false, -1)), input[0, array<int>, true], None).toIntArray
[info]   at org.apache.spark.sql.catalyst.encoders.ExpressionEncoder$Deserializer.apply(ExpressionEncoder.scala:186)
[info]   at org.apache.spark.sql.catalyst.encoders.ExpressionEncoderSuite.$anonfun$encodeDecodeTest$1(ExpressionEncoderSuite.scala:576)
[info]   at org.apache.spark.sql.catalyst.encoders.ExpressionEncoderSuite.verifyNotLeakingReflectionObjects(ExpressionEncoderSuite.scala:656)
[info]   at org.apache.spark.sql.catalyst.encoders.ExpressionEncoderSuite.$anonfun$testAndVerifyNotLeakingReflectionObjects$2(ExpressionEncoderSuite.scala:669)

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test suite fix aims to land to master/3.1/3.0/2.4.

Copy link
Contributor

@cloud-fan cloud-fan Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, let's fix these bugs one by one later.

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")
Expand All @@ -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)
Expand All @@ -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 {
Expand Down Expand Up @@ -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))
Expand All @@ -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))

Expand All @@ -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"))))
Expand Down Expand Up @@ -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")

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
}
}
}
}
18 changes: 12 additions & 6 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/PlanTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

@cloud-fan cloud-fan Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super.test sets the sql conf again?

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

withSQLConf depends on SQLConf.get. The existing code cannot guarantee the configuration setting of line 47 at super.test's execution time.

Copy link
Contributor

@cloud-fan cloud-fan Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SQLConf is thread-local. Does super.test runs in another thread by the SBT test runner?

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we define these tests by using super.test instead of executing.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the derived classes define test cases with this test, two test cases are created but withSQLConf is not inside the defined tests.

Copy link
Contributor

@cloud-fan cloud-fan Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, now I got it! What a hidden bug 😂

}
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)
}
}

Expand Down