[SPARK-44039][CONNECT][TESTS] Improve for PlanGenerationTestSuite & ProtoToParsedPlanTestSuite #41572

panbingkun · 2023-06-13T12:02:43Z

What changes were proposed in this pull request?

The pr aims to improve for PlanGenerationTestSuite & ProtoToParsedPlanTestSuite, include:

When generating GOLDEN files, we should first delete the corresponding directories and generate new ones to avoid submitting some redundant files during the review process. eg:
When we write a test named make_timestamp_ltz for the overloaded method, and during the review process, the reviewer wishes to add more tests for the method. The name of this method has changed during the next submission process, such as make_timestamp_ltz without timezone.At this point, if the queries/function_make_timestamp_ltz.json, queries/function_make_timestamp_ltz.proto.bin and explain-results/function_make_timestamp_ltz.explain files of function_make_timestamp_ltz are already in the commit, and there are many of these files, we generally do not notice the above problem, which leads to the incorrect submission of queries/function_make_timestamp_ltz.json, queries/function_make_timestamp_ltz.proto.bin and explain-results/function_make_timestamp_ltz.explain files without any impact on UT. These files are redundant.
Clear and update some redundant files submitted incorrectly

Why are the changes needed?

Make code clear.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass GA.

…rotoToParsedPlanTestSuite

panbingkun · 2023-06-13T12:04:37Z

connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/PlanGenerationTestSuite.scala

@@ -103,6 +104,12 @@ class PlanGenerationTestSuite
    val client = SparkConnectClient(InProcessChannelBuilder.forName("/dev/null").build())
    session =
      new SparkSession(client, cleaner = SparkSession.cleaner, planIdGenerator = new AtomicLong)
+    if (regenerateGoldenFiles) {


A simple method is to delete all the corresponding directories when generating golden files. This way, in our logic, we won't accidentally submit redundant files.

Good catch! Thank you ! Personally, I feet delete all golden files and regenerate is OK.

panbingkun · 2023-06-13T12:05:20Z

@HyukjinKwon @zhengruifeng @LuciferYang @beliefer

LuciferYang · 2023-06-13T12:25:22Z

connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/PlanGenerationTestSuite.scala

@@ -103,6 +104,12 @@ class PlanGenerationTestSuite
    val client = SparkConnectClient(InProcessChannelBuilder.forName("/dev/null").build())
    session =
      new SparkSession(client, cleaner = SparkSession.cleaner, planIdGenerator = new AtomicLong)
+    if (regenerateGoldenFiles) {
+      if (queryFilePath.toFile.exists()) {


nit: maybe we can use Utils.deleteRecursively

…rotoToParsedPlanTestSuite

zhengruifeng · 2023-06-13T23:53:16Z

cc @hvanhovell

beliefer · 2023-06-14T02:14:27Z

connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/PlanGenerationTestSuite.scala

@@ -103,6 +105,12 @@ class PlanGenerationTestSuite
    val client = SparkConnectClient(InProcessChannelBuilder.forName("/dev/null").build())
    session =
      new SparkSession(client, cleaner = SparkSession.cleaner, planIdGenerator = new AtomicLong)
+    if (regenerateGoldenFiles) {
+      if (queryFilePath.toFile.exists()) {


Please extract queryFilePath.toFile as a variable like val ...Path = queryFilePath.toFile

Done,
Meanwhile, let's verify #41579.

beliefer · 2023-06-14T02:14:59Z

.../connect/server/src/test/scala/org/apache/spark/sql/connect/ProtoToParsedPlanTestSuite.scala

@@ -90,6 +92,13 @@ class ProtoToParsedPlanTestSuite
          " theid INTEGER, \"Dept\" INTEGER)")
      .executeUpdate()
    conn.commit()
+
+    if (regenerateGoldenFiles) {
+      if (goldenFilePath.toFile.exists()) {


…rotoToParsedPlanTestSuite

beliefer · 2023-06-14T02:42:07Z

connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/PlanGenerationTestSuite.scala

+    val queryFile = queryFilePath.toFile
    if (regenerateGoldenFiles) {


Suggested change

val queryFile = queryFilePath.toFile

if (regenerateGoldenFiles) {

if (regenerateGoldenFiles) {

val queryFile = queryFilePath.toFile

beliefer · 2023-06-14T02:43:07Z

.../connect/server/src/test/scala/org/apache/spark/sql/connect/ProtoToParsedPlanTestSuite.scala

+    val goldenFile = goldenFilePath.toFile
+    if (regenerateGoldenFiles) {


Suggested change

val goldenFile = goldenFilePath.toFile

if (regenerateGoldenFiles) {

if (regenerateGoldenFiles) {

val goldenFile = goldenFilePath.toFile

…rotoToParsedPlanTestSuite

beliefer · 2023-06-14T11:59:21Z

LGTM.

zhengruifeng · 2023-06-15T02:47:10Z

SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "connect-client-jvm/testOnly org.apache.spark.sql.PlanGenerationTestSuite -- -z lpad"
...

[info] PlanGenerationTestSuite:
[info] - function lpad (35 milliseconds)
[info] - function lpad binary (1 millisecond)
[info] Run completed in 2 seconds, 58 milliseconds.
[info] Total number of tests run: 2
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 2, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[success] Total time: 120 s (02:00), completed Jun 15, 2023, 10:42:52 AM

will re-generating golden files for single test or a group of tests still be supported after this PR?

panbingkun · 2023-06-15T03:34:07Z

SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "connect-client-jvm/testOnly org.apache.spark.sql.PlanGenerationTestSuite -- -z lpad"
...

[info] PlanGenerationTestSuite:
[info] - function lpad (35 milliseconds)
[info] - function lpad binary (1 millisecond)
[info] Run completed in 2 seconds, 58 milliseconds.
[info] Total number of tests run: 2
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 2, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[success] Total time: 120 s (02:00), completed Jun 15, 2023, 10:42:52 AM

will re-generating golden files for single test or a group of tests still be supported after this PR?

No longer supported, Let me improve it again.

hvanhovell · 2023-06-15T03:52:18Z

I have another concern, for testing backwards compatibility it might be useful to keep 'orphaned' protos around. This would effectively kill that.

…rotoToParsedPlanTestSuite

panbingkun · 2023-06-24T12:12:40Z

SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "connect-client-jvm/testOnly org.apache.spark.sql.PlanGenerationTestSuite -- -z lpad"
...

[info] PlanGenerationTestSuite:
[info] - function lpad (35 milliseconds)
[info] - function lpad binary (1 millisecond)
[info] Run completed in 2 seconds, 58 milliseconds.
[info] Total number of tests run: 2
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 2, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[success] Total time: 120 s (02:00), completed Jun 15, 2023, 10:42:52 AM

will re-generating golden files for single test or a group of tests still be supported after this PR?

@zhengruifeng
The current logic already supports the above scenario.

panbingkun · 2023-06-24T12:17:38Z

I have another concern, for testing backwards compatibility it might be useful to keep 'orphaned' protos around. This would effectively kill that.

@hvanhovell

1.Very good reminder, but currently the orphan files deleted from the above, such as function_percentile.proto.bin, function_regexp_extract_all.proto.bin,function_regexp_instr.proto.bin, these are all files submitted by mistake during the review process.

2.At the same time, I have added additional explanatory notes in the code comments.
https://github.com/apache/spark/pull/41572/files#diff-a2b06ed831053e086e47d191c1e86d26da81c2d2a5cbaedd34a712297a2b418dR70

3.We should provide an automated function to find orphaned files, otherwise, finding files that can be deleted from over 600 files may be a bit difficult. As for whether to delete them, I think it needs to be weighed between the submitter and the code reviewer. Otherwise, many orphaned files are increasing, and many of them are only generated by submitting them incorrectly. For example, from the first submission of this PR to today, two orphan files have been generated: function_regexp_extract_all.proto.bin, function_regexp_instr.proto.bin.

…rotoToParsedPlanTestSuite

hvanhovell · 2023-06-28T01:36:48Z

Alright works for me.

I guess orphaned protos will show up in the git diff now. I guess we can create a separate folder for the ones we want to keep.

hvanhovell

LGTM

hvanhovell · 2023-06-28T01:37:15Z

Merging

HyukjinKwon

LGTM2

[SPARK-44039][CONNECT][TESTS] Improve for PlanGenerationTestSuite & P…

20b0b40

…rotoToParsedPlanTestSuite

github-actions bot added CONNECT SQL labels Jun 13, 2023

panbingkun commented Jun 13, 2023

View reviewed changes

LuciferYang reviewed Jun 13, 2023

View reviewed changes

[SPARK-44039][CONNECT][TESTS] Improve for PlanGenerationTestSuite & P…

7636282

…rotoToParsedPlanTestSuite

LuciferYang approved these changes Jun 13, 2023

View reviewed changes

beliefer reviewed Jun 14, 2023

View reviewed changes

panbingkun added 2 commits June 14, 2023 10:28

Merge branch 'master' into SPARK-44039

af9f94b

[SPARK-44039][CONNECT][TESTS] Improve for PlanGenerationTestSuite & P…

840c532

…rotoToParsedPlanTestSuite

beliefer reviewed Jun 14, 2023

View reviewed changes

[SPARK-44039][CONNECT][TESTS] Improve for PlanGenerationTestSuite & P…

34b665f

…rotoToParsedPlanTestSuite

panbingkun added 2 commits June 24, 2023 09:46

Merge branch 'master' into SPARK-44039

163dbde

[SPARK-44039][CONNECT][TESTS] Improve for PlanGenerationTestSuite & P…

00536d5

…rotoToParsedPlanTestSuite

[SPARK-44039][CONNECT][TESTS] Improve for PlanGenerationTestSuite & P…

96eaa48

…rotoToParsedPlanTestSuite

hvanhovell approved these changes Jun 28, 2023

View reviewed changes

hvanhovell closed this in c8c51d0 Jun 28, 2023

HyukjinKwon reviewed Jul 3, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-44039][CONNECT][TESTS] Improve for PlanGenerationTestSuite & ProtoToParsedPlanTestSuite #41572

[SPARK-44039][CONNECT][TESTS] Improve for PlanGenerationTestSuite & ProtoToParsedPlanTestSuite #41572

panbingkun commented Jun 13, 2023 •

edited

panbingkun Jun 13, 2023

beliefer Jun 13, 2023

panbingkun commented Jun 13, 2023

LuciferYang Jun 13, 2023

panbingkun Jun 13, 2023

zhengruifeng commented Jun 13, 2023

beliefer Jun 14, 2023 •

edited

panbingkun Jun 14, 2023

beliefer Jun 14, 2023

beliefer Jun 14, 2023

beliefer Jun 14, 2023

beliefer commented Jun 14, 2023

zhengruifeng commented Jun 15, 2023

panbingkun commented Jun 15, 2023

hvanhovell commented Jun 15, 2023

panbingkun commented Jun 24, 2023 •

edited

panbingkun commented Jun 24, 2023 •

edited

hvanhovell commented Jun 28, 2023

hvanhovell left a comment

hvanhovell commented Jun 28, 2023

HyukjinKwon left a comment

		val queryFile = queryFilePath.toFile
		if (regenerateGoldenFiles) {

		val goldenFile = goldenFilePath.toFile
		if (regenerateGoldenFiles) {

[SPARK-44039][CONNECT][TESTS] Improve for PlanGenerationTestSuite & ProtoToParsedPlanTestSuite #41572

[SPARK-44039][CONNECT][TESTS] Improve for PlanGenerationTestSuite & ProtoToParsedPlanTestSuite #41572

Conversation

panbingkun commented Jun 13, 2023 • edited

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panbingkun commented Jun 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengruifeng commented Jun 13, 2023

beliefer Jun 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beliefer commented Jun 14, 2023

zhengruifeng commented Jun 15, 2023

panbingkun commented Jun 15, 2023

hvanhovell commented Jun 15, 2023

panbingkun commented Jun 24, 2023 • edited

panbingkun commented Jun 24, 2023 • edited

hvanhovell commented Jun 28, 2023

hvanhovell left a comment

Choose a reason for hiding this comment

hvanhovell commented Jun 28, 2023

HyukjinKwon left a comment

Choose a reason for hiding this comment

panbingkun commented Jun 13, 2023 •

edited

beliefer Jun 14, 2023 •

edited

panbingkun commented Jun 24, 2023 •

edited

panbingkun commented Jun 24, 2023 •

edited