Skip to content

Conversation

@LuciferYang
Copy link
Contributor

@LuciferYang LuciferYang commented Aug 16, 2023

What changes were proposed in this pull request?

This PR refactors the test case REPL class in UDF to ensure that Scala 2.12 and Scala 2.13 use the same results for assertion.

Why are the changes needed?

There are the following test failures in the daily testing of branch-3.5:

image

Does this PR introduce any user-facing change?

No

How was this patch tested?

  • Pass Github Actions
  • Manually check
dev/change-scala-version.sh 2.13
build/sbt clean "connect-client-jvm/testOnly org.apache.spark.sql.application.ReplE2ESuite" -Phive -Pscala-2.13

Before

[info]   @ case class MyTestClass(value: Int) 
defined class MyTestClass
[info]   
[info]   @ spark.range(2).map(i => MyTestClass(i.toInt)).collect() 
res56: Array[MyTestClass] = Array(MyTestClass(value = 0), MyTestClass(value = 1))
[info]   
[info]   @        

[info]   @ semaphore.release() 

[info]   Error Output: Compiling (synthetic)/ammonite/predef/ArgsPredef.sc
[info]   Compiling /Users/yangjie01/SourceCode/git/spark-mine-sbt/connector/connect/client/jvm/(console) (ReplE2ESuite.scala:111)
[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.Assertions$.newAssertionFailedException(Assertions.scala:1231)
[info]   at org.scalatest.Assertions$AssertionsHelper.macroAssert(Assertions.scala:1295)
[info]   at org.apache.spark.sql.application.ReplE2ESuite.assertContains(ReplE2ESuite.scala:111)
[info]   at org.apache.spark.sql.application.ReplE2ESuite.$anonfun$new$14(ReplE2ESuite.scala:289)
[info]   at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)
[info]   at org.apache.spark.sql.connect.client.util.RemoteSparkSession.$anonfun$test$1(RemoteSparkSession.scala:243)
...
[info] Run completed in 41 seconds, 573 milliseconds.
[info] Total number of tests run: 15
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 14, failed 1, canceled 0, ignored 0, pending 0
[info] *** 1 TEST FAILED ***

After

[info] ReplE2ESuite:
[info] - Simple query (4 seconds, 334 milliseconds)
[info] - UDF containing 'def' (1 second, 509 milliseconds)
[info] - UDF containing in-place lambda (897 milliseconds)
[info] - Updating UDF properties (904 milliseconds)
[info] - SPARK-43198: Filter does not throw ammonite-related class initialization exception (1 second, 137 milliseconds)
[info] - Client-side JAR (1 second, 329 milliseconds)
[info] - Java UDF (950 milliseconds)
[info] - Java UDF Registration (950 milliseconds)
[info] - UDF Registration (932 milliseconds)
[info] - UDF closure registration (807 milliseconds)
[info] - call_udf (1 second, 235 milliseconds)
[info] - call_function (884 milliseconds)
[info] - Collect REPL generated class (1 second, 198 milliseconds)
[info] - REPL class in UDF (732 milliseconds)
[info] - streaming works with REPL generated code (2 seconds, 845 milliseconds)
[info] Run completed in 41 seconds, 346 milliseconds.
[info] Total number of tests run: 15
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 15, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.

@LuciferYang LuciferYang changed the title [SPARK-44795][CONNECT][TESTS][FOLLOWUP] Fix test case Collect REPL generated class for Scala 2.13 [SPARK-44795][CONNECT][TESTS][FOLLOWUP] Fix test case REPL class in UDF for Scala 2.13 Aug 16, 2023
""".stripMargin
val output = runCommandsInShell(input)
assertContains("Array[MyTestClass] = Array(MyTestClass(0), MyTestClass(1))", output)
assertContains("""String = "[MyTestClass(0), MyTestClass(1)]"""", output)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @hvanhovell similar as #42485, REPL class in UDF also need fix for Scala 2.13 ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't Scala 2.13 print this as String = "[MyTestClass(value = 0), MyTestClass(value = 1)]"?

Copy link
Contributor

@hvanhovell hvanhovell left a comment

Choose a reason for hiding this comment

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

LGTM

LuciferYang added a commit that referenced this pull request Aug 16, 2023
…UDF` for Scala 2.13

### What changes were proposed in this pull request?
This PR refactors the test case `REPL class in UDF` to ensure that Scala 2.12 and Scala 2.13 use the same results for assertion.

### Why are the changes needed?
There are the following test failures in the daily testing of branch-3.5:

- https://github.com/apache/spark/actions/runs/5866399652/job/15905157665

<img width="941" alt="image" src="https://github.com/apache/spark/assets/1475305/62dcd4eb-e01f-4a11-bf17-386129fdb08d">

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
- Pass Github Actions
- Manually check

```
dev/change-scala-version.sh 2.13
build/sbt clean "connect-client-jvm/testOnly org.apache.spark.sql.application.ReplE2ESuite" -Phive -Pscala-2.13
```

**Before**

```
[info]    case class MyTestClass(value: Int)
defined class MyTestClass
[info]
[info]    spark.range(2).map(i => MyTestClass(i.toInt)).collect()
res56: Array[MyTestClass] = Array(MyTestClass(value = 0), MyTestClass(value = 1))
[info]
[info]

[info]    semaphore.release()

[info]   Error Output: Compiling (synthetic)/ammonite/predef/ArgsPredef.sc
[info]   Compiling /Users/yangjie01/SourceCode/git/spark-mine-sbt/connector/connect/client/jvm/(console) (ReplE2ESuite.scala:111)
[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.Assertions$.newAssertionFailedException(Assertions.scala:1231)
[info]   at org.scalatest.Assertions$AssertionsHelper.macroAssert(Assertions.scala:1295)
[info]   at org.apache.spark.sql.application.ReplE2ESuite.assertContains(ReplE2ESuite.scala:111)
[info]   at org.apache.spark.sql.application.ReplE2ESuite.$anonfun$new$14(ReplE2ESuite.scala:289)
[info]   at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)
[info]   at org.apache.spark.sql.connect.client.util.RemoteSparkSession.$anonfun$test$1(RemoteSparkSession.scala:243)
...
[info] Run completed in 41 seconds, 573 milliseconds.
[info] Total number of tests run: 15
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 14, failed 1, canceled 0, ignored 0, pending 0
[info] *** 1 TEST FAILED ***
```

**After**

```
[info] ReplE2ESuite:
[info] - Simple query (4 seconds, 334 milliseconds)
[info] - UDF containing 'def' (1 second, 509 milliseconds)
[info] - UDF containing in-place lambda (897 milliseconds)
[info] - Updating UDF properties (904 milliseconds)
[info] - SPARK-43198: Filter does not throw ammonite-related class initialization exception (1 second, 137 milliseconds)
[info] - Client-side JAR (1 second, 329 milliseconds)
[info] - Java UDF (950 milliseconds)
[info] - Java UDF Registration (950 milliseconds)
[info] - UDF Registration (932 milliseconds)
[info] - UDF closure registration (807 milliseconds)
[info] - call_udf (1 second, 235 milliseconds)
[info] - call_function (884 milliseconds)
[info] - Collect REPL generated class (1 second, 198 milliseconds)
[info] - REPL class in UDF (732 milliseconds)
[info] - streaming works with REPL generated code (2 seconds, 845 milliseconds)
[info] Run completed in 41 seconds, 346 milliseconds.
[info] Total number of tests run: 15
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 15, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
 ```

Closes #42508 from LuciferYang/SPARK-44795-FOLLOWUP.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: yangjie01 <yangjie01@baidu.com>
(cherry picked from commit ed906e0)
Signed-off-by: yangjie01 <yangjie01@baidu.com>
@LuciferYang
Copy link
Contributor Author

Merged into master and branch-3.5. Thanks @hvanhovell

valentinp17 pushed a commit to valentinp17/spark that referenced this pull request Aug 24, 2023
…UDF` for Scala 2.13

### What changes were proposed in this pull request?
This PR refactors the test case `REPL class in UDF` to ensure that Scala 2.12 and Scala 2.13 use the same results for assertion.

### Why are the changes needed?
There are the following test failures in the daily testing of branch-3.5:

- https://github.com/apache/spark/actions/runs/5866399652/job/15905157665

<img width="941" alt="image" src="https://github.com/apache/spark/assets/1475305/62dcd4eb-e01f-4a11-bf17-386129fdb08d">

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
- Pass Github Actions
- Manually check

```
dev/change-scala-version.sh 2.13
build/sbt clean "connect-client-jvm/testOnly org.apache.spark.sql.application.ReplE2ESuite" -Phive -Pscala-2.13
```

**Before**

```
[info]    case class MyTestClass(value: Int)
defined class MyTestClass
[info]
[info]    spark.range(2).map(i => MyTestClass(i.toInt)).collect()
res56: Array[MyTestClass] = Array(MyTestClass(value = 0), MyTestClass(value = 1))
[info]
[info]

[info]    semaphore.release()

[info]   Error Output: Compiling (synthetic)/ammonite/predef/ArgsPredef.sc
[info]   Compiling /Users/yangjie01/SourceCode/git/spark-mine-sbt/connector/connect/client/jvm/(console) (ReplE2ESuite.scala:111)
[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.Assertions$.newAssertionFailedException(Assertions.scala:1231)
[info]   at org.scalatest.Assertions$AssertionsHelper.macroAssert(Assertions.scala:1295)
[info]   at org.apache.spark.sql.application.ReplE2ESuite.assertContains(ReplE2ESuite.scala:111)
[info]   at org.apache.spark.sql.application.ReplE2ESuite.$anonfun$new$14(ReplE2ESuite.scala:289)
[info]   at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)
[info]   at org.apache.spark.sql.connect.client.util.RemoteSparkSession.$anonfun$test$1(RemoteSparkSession.scala:243)
...
[info] Run completed in 41 seconds, 573 milliseconds.
[info] Total number of tests run: 15
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 14, failed 1, canceled 0, ignored 0, pending 0
[info] *** 1 TEST FAILED ***
```

**After**

```
[info] ReplE2ESuite:
[info] - Simple query (4 seconds, 334 milliseconds)
[info] - UDF containing 'def' (1 second, 509 milliseconds)
[info] - UDF containing in-place lambda (897 milliseconds)
[info] - Updating UDF properties (904 milliseconds)
[info] - SPARK-43198: Filter does not throw ammonite-related class initialization exception (1 second, 137 milliseconds)
[info] - Client-side JAR (1 second, 329 milliseconds)
[info] - Java UDF (950 milliseconds)
[info] - Java UDF Registration (950 milliseconds)
[info] - UDF Registration (932 milliseconds)
[info] - UDF closure registration (807 milliseconds)
[info] - call_udf (1 second, 235 milliseconds)
[info] - call_function (884 milliseconds)
[info] - Collect REPL generated class (1 second, 198 milliseconds)
[info] - REPL class in UDF (732 milliseconds)
[info] - streaming works with REPL generated code (2 seconds, 845 milliseconds)
[info] Run completed in 41 seconds, 346 milliseconds.
[info] Total number of tests run: 15
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 15, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
 ```

Closes apache#42508 from LuciferYang/SPARK-44795-FOLLOWUP.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: yangjie01 <yangjie01@baidu.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants