[SPARK-42599][CONNECT][INFRA] Introduce `dev/connect-jvm-client-mima-check` instead of `CompatibilitySuite` #40213

LuciferYang · 2023-02-28T07:29:42Z

What changes were proposed in this pull request?

The main changes of this pr as follows:

Refactor CompatibilitySuite as a new tool CheckConnectJvmClientCompatibility
Add a new shell script dev/connect-jvm-client-mima-check, it will use CheckConnectJvmClientCompatibility to check the mima compatibility of connect-jvm-client module.
Add dev/connect-jvm-client-mima-check to github task

Why are the changes needed?

For fix test error report in [VOTE] Release Apache Spark 3.4.0 (RC1) mail list.

Testing CompatibilitySuite with maven requires some pre-work:

build/mvn clean install -DskipTests -pl sql/core -am
build/mvn clean install -DskipTests -pl connector/connect/client/jvm -am

So if we run build/mvn package test to test whole project as before, CompatibilitySuite will failed as follows:

CompatibilitySuite:
- compatibility MiMa tests *** FAILED ***
  java.lang.AssertionError: assertion failed: Failed to find the jar inside folder: /home/bjorn/spark-3.4.0/connector/connect/client/jvm/target
  at scala.Predef$.assert(Predef.scala:223)
  at org.apache.spark.sql.connect.client.util.IntegrationTestUtils$.findJar(IntegrationTestUtils.scala:67)
  at org.apache.spark.sql.connect.client.CompatibilitySuite.clientJar$lzycompute(CompatibilitySuite.scala:57)
  at org.apache.spark.sql.connect.client.CompatibilitySuite.clientJar(CompatibilitySuite.scala:53)
  at org.apache.spark.sql.connect.client.CompatibilitySuite.$anonfun$new$1(CompatibilitySuite.scala:69)
  at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85)
  at org.scalatest.OutcomeOf.outcomeOf$(OutcomeOf.scala:83)
  at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104)
  at org.scalatest.Transformer.apply(Transformer.scala:22)
  at org.scalatest.Transformer.apply(Transformer.scala:20)
  ...
- compatibility API tests: Dataset *** FAILED ***
  java.lang.AssertionError: assertion failed: Failed to find the jar inside folder: /home/bjorn/spark-3.4.0/connector/connect/client/jvm/target
  at scala.Predef$.assert(Predef.scala:223)
  at org.apache.spark.sql.connect.client.util.IntegrationTestUtils$.findJar(IntegrationTestUtils.scala:67)
  at org.apache.spark.sql.connect.client.CompatibilitySuite.clientJar$lzycompute(CompatibilitySuite.scala:57)
  at org.apache.spark.sql.connect.client.CompatibilitySuite.clientJar(CompatibilitySuite.scala:53)
  at org.apache.spark.sql.connect.client.CompatibilitySuite.$anonfun$new$7(CompatibilitySuite.scala:110)
  at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23)
  at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85)
  at org.scalatest.OutcomeOf.outcomeOf$(OutcomeOf.scala:83)
  at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104)
  at org.scalatest.Transformer.apply(Transformer.scala:22)

So we need to fix this problem for developers.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass GitHub Actions

LuciferYang · 2023-02-28T08:19:03Z

.../src/test/scala/org/apache/spark/sql/connect/client/CheckConnectJvmClientCompatibility.scala

-
-    val clientClass = clientClassLoader.loadClass("org.apache.spark.sql.Dataset")
-    val sqlClass = sqlClassLoader.loadClass("org.apache.spark.sql.Dataset")
+  private def checkDatasetApiCompatibility(clientJar: File, sqlJar: File): Seq[String] = {


Note: When CheckConnectJvmClientCompatibility in the connect/jvm module, this check will fail due to #39712 (comment)

Hi, I would recommend to use this PR. For this failing tests. Let's just delete this. I was planning to delete it after this PR get merged. The test case is already covered by mima test. Thanks so much. This change looks awesome.

zhenlineo · 2023-02-28T20:59:36Z

LGTM

hvanhovell · 2023-03-01T16:19:11Z

dev/connect-jvm-client-mima-check

+rm -f .connect-mima-check-result
+
+echo "Build sql module ..."
+build/sbt sql/package


You can issue a single command here, that will be a bit faster:

build/sbt "sql/package;connect-client-jvm/assembly;connect-client-jvm/Test/package"

Thanks ~ fixed

hvanhovell · 2023-03-01T16:19:51Z

dev/connect-jvm-client-mima-check

+
+CONNECT_TEST_CLASSPATH="$(build/sbt -DcopyDependencies=false "export connect-client-jvm/Test/fullClasspath" | grep jar | tail -n1)"
+CONNECT_CLASSPATH="$(build/sbt -DcopyDependencies=false "export connect-client-jvm/fullClasspath" | grep jar | tail -n1)"
+SQL_CLASSPATH="$(build/sbt -DcopyDependencies=false "export sql/fullClasspath" | grep jar | tail -n1)"


There will be duplication of entries here. Is that an issue?

Yes, there will be duplicate entries, but there is no problem for the time being,

hvanhovell · 2023-03-01T16:21:26Z

connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/Dataset.scala

  }

-  def collectResult(): SparkResult[T] = sparkSession.execute(plan, encoder)
+  private[sql] def collectResult(): SparkResult[T] = sparkSession.execute(plan, encoder)


Why make this private[sql]? For advanced use cases this is a better way of interacting with results.

I guess it is because this is not an existing API?

I don't have a concrete idea now how to deal with new public API that is introduced by Spark Connect.

Maybe we do not expose such API for now given that they are new, and later change it to public if that is useful. I assume it will be hard to do the reversed way which becomes breaking changes.

checkDatasetApiCompatibility('compatibility API tests: Dataset' in CompatibilitySuite) will find out the public Dataset api in connect/client module instead of in sql module. collectResult is such a case. There was no problem before because there was bad case in the test (classloader loaded the wrong class)

I guess it is because this is not an existing API?

Yes

I think If we accept similar changes (add some new public apis to connect/client module), we may need to maintain a whitelist(in checkDatasetApiCompatibility) or do not check this again

// For now we simply check the new methods is a subset of the old methods. clientMethods.diff(sqlMethods)

the new methods is a subset of the old method -> clientMethods.diff(sqlMethods) should be empty?

For example:

scala> val clientMethods = Seq(1,2,3) clientMethods: Seq[Int] = List(1, 2, 3) scala> val sqlMethods = Seq(1,2,3,4,5) sqlMethods: Seq[Int] = List(1, 2, 3, 4, 5) scala> clientMethods.diff(sqlMethods) res0: Seq[Int] = List()

clientMethods is a subset of sqlMethods.

scala> val clientMethods = Seq(0,1,2,3) clientMethods: Seq[Int] = List(0, 1, 2, 3) scala> val sqlMethods = Seq(1,2,3,4,5) sqlMethods: Seq[Int] = List(1, 2, 3, 4, 5) scala> clientMethods.diff(sqlMethods) res0: Seq[Int] = List(0)

clientMethods is not a subset of sqlMethods.

The original test code as follows. I think it is consistent with the current intention, am I right?

// For now we simply check the new methods is a subset of the old methods. newMethods .map(m => m.toString) .foreach(method => { assert(oldMethods.map(m => m.toString).contains(method)) })

Please feel free to ignore me and work with @hvanhovell on this case. :)

@LuciferYang can we make an exception for the SparkResult class. Basically filter out the DataSet.collectResult method. That is all.

let me add a exceptionMethods to checkDatasetApiCompatibility

hvanhovell · 2023-03-01T16:22:18Z

@LuciferYang thanks for the hard work! Can we merge this one instead of #40191.

amaliujia · 2023-03-01T21:43:21Z

Looks awesome overall!

amaliujia · 2023-03-01T21:46:28Z

How would we deal with CompatibilitySuite?

amaliujia · 2023-03-01T21:57:47Z

Ah I see now that we still need to update the excluding rules.

zhenlineo · 2023-03-01T22:19:35Z

.../src/test/scala/org/apache/spark/sql/connect/client/CheckConnectJvmClientCompatibility.scala

+  private def checkDatasetApiCompatibility(clientJar: File, sqlJar: File): Seq[String] = {
+
+    def methods(jar: File, className: String): Seq[String] = {
+      val classLoader: URLClassLoader = new URLClassLoader(Seq(jar.toURI.toURL).toArray)


To be more safe about the class is loaded correctly. We can also use this class loader:

import org.apache.spark.util.ChildFirstURLClassLoader val classLoader: URLClassLoader = new ChildFirstURLClassLoader(Seq(jar.toURI.toURL).toArray, this.getClass.getClassLoader)

This is effective, the target class can also be loaded correctly in the connect module.
I checked it manually： removed the change of Dataset.scala and run dev/connect-jvm-client-mima-check, it can find out org.apache.spark.sql.Dataset.collectResult not exist in sql module.

Thanks @zhenlineo

And I do manually check for the original test compatibility API tests: Dataset, it also became failed:

failed.log (The error message is too large. I pasted it into the file)

LuciferYang · 2023-03-02T00:39:35Z

How would we deal with CompatibilitySuite
Ah I see now that we still need to update the excluding rules.

Yes, just check in a different way :)

LuciferYang · 2023-03-02T03:45:08Z

The new change of org.apache.spark.sql.Dataset#plan in SPARK-42631 is checked as incompatible.

LuciferYang · 2023-03-02T04:06:57Z

.../src/test/scala/org/apache/spark/sql/connect/client/CheckConnectJvmClientCompatibility.scala

+    val clientMethods = methods(clientJar, className)
+    val sqlMethods = methods(sqlJar, className)
+    // Exclude some public methods that must be added through `exceptions`
+    val exceptionMethods =


@hvanhovell exceptionMethods has been added and the revert the change of Dataset. Can we simply fix it like this first?

If I can refine this check, I will give followup later

hvanhovell

LGTM

hvanhovell · 2023-03-02T04:28:38Z

.../src/test/scala/org/apache/spark/sql/connect/client/CheckConnectJvmClientCompatibility.scala

-  test("compatibility API tests: Dataset") {
-    val clientClassLoader: URLClassLoader = new URLClassLoader(Seq(clientJar.toURI.toURL).toArray)
-    val sqlClassLoader: URLClassLoader = new URLClassLoader(Seq(sqlJar.toURI.toURL).toArray)
+  private def checkDatasetApiCompatibility(clientJar: File, sqlJar: File): Seq[String] = {


@zhenlineo is this only supposed to check Dataset? what about the other classes?

This test was initially added because we did not cover the Dataset in the mima check.

If we found such test is useful and want to expand to more classes we can run the mima check the other way around to ensure that our new code does not introduce more unwanted methods.

hvanhovell · 2023-03-02T18:35:46Z

Alirght merging.

…check` instead of `CompatibilitySuite` ### What changes were proposed in this pull request? The main changes of this pr as follows: - Refactor `CompatibilitySuite` as a new tool `CheckConnectJvmClientCompatibility` - Add a new shell script `dev/connect-jvm-client-mima-check`, it will use `CheckConnectJvmClientCompatibility` to check the mima compatibility of `connect-jvm-client` module. - Add `dev/connect-jvm-client-mima-check` to github task ### Why are the changes needed? For fix test error report in `[VOTE] Release Apache Spark 3.4.0 (RC1)` mail list. Testing `CompatibilitySuite` with maven requires some pre-work: ``` build/mvn clean install -DskipTests -pl sql/core -am build/mvn clean install -DskipTests -pl connector/connect/client/jvm -am ``` So if we run `build/mvn package test` to test whole project as before, `CompatibilitySuite` will failed as follows: ``` CompatibilitySuite: - compatibility MiMa tests *** FAILED *** java.lang.AssertionError: assertion failed: Failed to find the jar inside folder: /home/bjorn/spark-3.4.0/connector/connect/client/jvm/target at scala.Predef$.assert(Predef.scala:223) at org.apache.spark.sql.connect.client.util.IntegrationTestUtils$.findJar(IntegrationTestUtils.scala:67) at org.apache.spark.sql.connect.client.CompatibilitySuite.clientJar$lzycompute(CompatibilitySuite.scala:57) at org.apache.spark.sql.connect.client.CompatibilitySuite.clientJar(CompatibilitySuite.scala:53) at org.apache.spark.sql.connect.client.CompatibilitySuite.$anonfun$new$1(CompatibilitySuite.scala:69) at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85) at org.scalatest.OutcomeOf.outcomeOf$(OutcomeOf.scala:83) at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104) at org.scalatest.Transformer.apply(Transformer.scala:22) at org.scalatest.Transformer.apply(Transformer.scala:20) ... - compatibility API tests: Dataset *** FAILED *** java.lang.AssertionError: assertion failed: Failed to find the jar inside folder: /home/bjorn/spark-3.4.0/connector/connect/client/jvm/target at scala.Predef$.assert(Predef.scala:223) at org.apache.spark.sql.connect.client.util.IntegrationTestUtils$.findJar(IntegrationTestUtils.scala:67) at org.apache.spark.sql.connect.client.CompatibilitySuite.clientJar$lzycompute(CompatibilitySuite.scala:57) at org.apache.spark.sql.connect.client.CompatibilitySuite.clientJar(CompatibilitySuite.scala:53) at org.apache.spark.sql.connect.client.CompatibilitySuite.$anonfun$new$7(CompatibilitySuite.scala:110) at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23) at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85) at org.scalatest.OutcomeOf.outcomeOf$(OutcomeOf.scala:83) at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104) at org.scalatest.Transformer.apply(Transformer.scala:22) ``` So we need to fix this problem for developers. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Pass GitHub Actions Closes #40213 from LuciferYang/connect-client-compatibility-backup. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Herman van Hovell <herman@databricks.com> (cherry picked from commit 12beadb) Signed-off-by: Herman van Hovell <herman@databricks.com>

LuciferYang · 2023-03-03T02:08:42Z

Thanks @hvanhovell @amaliujia @zhenlineo

…check` instead of `CompatibilitySuite` ### What changes were proposed in this pull request? The main changes of this pr as follows: - Refactor `CompatibilitySuite` as a new tool `CheckConnectJvmClientCompatibility` - Add a new shell script `dev/connect-jvm-client-mima-check`, it will use `CheckConnectJvmClientCompatibility` to check the mima compatibility of `connect-jvm-client` module. - Add `dev/connect-jvm-client-mima-check` to github task ### Why are the changes needed? For fix test error report in `[VOTE] Release Apache Spark 3.4.0 (RC1)` mail list. Testing `CompatibilitySuite` with maven requires some pre-work: ``` build/mvn clean install -DskipTests -pl sql/core -am build/mvn clean install -DskipTests -pl connector/connect/client/jvm -am ``` So if we run `build/mvn package test` to test whole project as before, `CompatibilitySuite` will failed as follows: ``` CompatibilitySuite: - compatibility MiMa tests *** FAILED *** java.lang.AssertionError: assertion failed: Failed to find the jar inside folder: /home/bjorn/spark-3.4.0/connector/connect/client/jvm/target at scala.Predef$.assert(Predef.scala:223) at org.apache.spark.sql.connect.client.util.IntegrationTestUtils$.findJar(IntegrationTestUtils.scala:67) at org.apache.spark.sql.connect.client.CompatibilitySuite.clientJar$lzycompute(CompatibilitySuite.scala:57) at org.apache.spark.sql.connect.client.CompatibilitySuite.clientJar(CompatibilitySuite.scala:53) at org.apache.spark.sql.connect.client.CompatibilitySuite.$anonfun$new$1(CompatibilitySuite.scala:69) at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85) at org.scalatest.OutcomeOf.outcomeOf$(OutcomeOf.scala:83) at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104) at org.scalatest.Transformer.apply(Transformer.scala:22) at org.scalatest.Transformer.apply(Transformer.scala:20) ... - compatibility API tests: Dataset *** FAILED *** java.lang.AssertionError: assertion failed: Failed to find the jar inside folder: /home/bjorn/spark-3.4.0/connector/connect/client/jvm/target at scala.Predef$.assert(Predef.scala:223) at org.apache.spark.sql.connect.client.util.IntegrationTestUtils$.findJar(IntegrationTestUtils.scala:67) at org.apache.spark.sql.connect.client.CompatibilitySuite.clientJar$lzycompute(CompatibilitySuite.scala:57) at org.apache.spark.sql.connect.client.CompatibilitySuite.clientJar(CompatibilitySuite.scala:53) at org.apache.spark.sql.connect.client.CompatibilitySuite.$anonfun$new$7(CompatibilitySuite.scala:110) at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23) at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85) at org.scalatest.OutcomeOf.outcomeOf$(OutcomeOf.scala:83) at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104) at org.scalatest.Transformer.apply(Transformer.scala:22) ``` So we need to fix this problem for developers. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Pass GitHub Actions Closes apache#40213 from LuciferYang/connect-client-compatibility-backup. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Herman van Hovell <herman@databricks.com> (cherry picked from commit 12beadb) Signed-off-by: Herman van Hovell <herman@databricks.com>

CompatibilitySuite to CheckConnectJvmClientCompatibility

658d0d0

LuciferYang marked this pull request as draft February 28, 2023 07:29

github-actions bot added BUILD CONNECT INFRA SQL labels Feb 28, 2023

LuciferYang mentioned this pull request Feb 28, 2023

[SPARK-42599][CONNECT][INFRA] Introduce dev/connect-jvm-client-mima-check instead of CompatibilitySuite #40191

Closed

LuciferYang commented Feb 28, 2023

View reviewed changes

fix format

6421468

zhenlineo approved these changes Feb 28, 2023

View reviewed changes

Merge branch 'upmaster' into connect-client-compatibility-backup

a895dfc

hvanhovell marked this pull request as ready for review March 1, 2023 16:16

hvanhovell reviewed Mar 1, 2023

View reviewed changes

zhenlineo reviewed Mar 1, 2023

View reviewed changes

LuciferYang added 2 commits March 2, 2023 08:21

Merge branch 'upmaster' into connect-client-compatibility-backup

82165ef

address comments

22f41fc

amaliujia mentioned this pull request Mar 2, 2023

[SPARK-42640][CONNECT] Remove stale entries from the excluding rules for CompatibilitySuite #40241

Closed

LuciferYang added 2 commits March 2, 2023 08:46

use ChildFirstURLClassLoader

c494fba

comments

8806856

LuciferYang added 2 commits March 2, 2023 11:49

change message

c140b95

add exceptionMethods

05367de

LuciferYang added 2 commits March 2, 2023 12:01

revert change of Dataset

6ccdb6b

comments

35e7b32

LuciferYang commented Mar 2, 2023

View reviewed changes

print \n

d4a05f1

hvanhovell approved these changes Mar 2, 2023

View reviewed changes

hvanhovell reviewed Mar 2, 2023

View reviewed changes

Merge branch 'upmaster' into connect-client-compatibility-backup

0f0934a

hvanhovell closed this in 12beadb Mar 2, 2023

[SPARK-42599][CONNECT][INFRA] Introduce dev/connect-jvm-client-mima-check instead of CompatibilitySuite #40213

[SPARK-42599][CONNECT][INFRA] Introduce dev/connect-jvm-client-mima-check instead of CompatibilitySuite #40213

Uh oh!

Conversation

LuciferYang commented Feb 28, 2023

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhenlineo commented Feb 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvanhovell Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuciferYang Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuciferYang Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amaliujia Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuciferYang Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvanhovell commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amaliujia commented Mar 1, 2023

Uh oh!

amaliujia commented Mar 1, 2023

Uh oh!

amaliujia commented Mar 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuciferYang Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuciferYang commented Mar 2, 2023

Uh oh!

LuciferYang commented Mar 2, 2023

Uh oh!

Choose a reason for hiding this comment

[SPARK-42599][CONNECT][INFRA] Introduce `dev/connect-jvm-client-mima-check` instead of `CompatibilitySuite` #40213

[SPARK-42599][CONNECT][INFRA] Introduce `dev/connect-jvm-client-mima-check` instead of `CompatibilitySuite` #40213

hvanhovell Mar 1, 2023 •

edited

Loading

LuciferYang Mar 2, 2023 •

edited

Loading

LuciferYang Mar 2, 2023 •

edited

Loading

amaliujia Mar 2, 2023 •

edited

Loading

LuciferYang Mar 2, 2023 •

edited

Loading

hvanhovell commented Mar 1, 2023 •

edited

Loading

LuciferYang Mar 2, 2023 •

edited

Loading