[SPARK-42791][SQL] Create a new golden file test framework for analysis #40449

dtenedor · 2023-03-15T18:19:15Z

What changes were proposed in this pull request?

This PR updates the SQLQueryTestSuite to also consume the same input SQL queries from the input files in second pass and then perform analysis and generate the string representation of the analyzed plans, in a separate new directory. It works much the same way as the previous SQLQueryTestSuite behavior except that it now also produces analyzed plan string instead of just query results in the output golden files.

Why are the changes needed?

This framework will help us guard against bugs in future development by showing a clear signal when analyzer updates result in changes to the query plans for a body of test queries. PR authors and reviewers will be able to see the diffs for all changed plans during the review, and any unintentional plan changes will act as a signal for PR authors to adjust their code to prevent the changes from happening.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

This test adds a new test suite and initial golden file. The new test suite passes as of this PR.

commit commit commit commit

dtenedor · 2023-03-15T19:34:01Z

Hi @gengliangwang this should be ready for a first look!

gengliangwang · 2023-03-15T20:33:32Z

@dtenedor Since we already have SQLQueryTestSuite which has good basic Spark SQL features coverage, shall we combine both? E.g. let SQLQueryTestSuite show analyzed plan/optimized plan/execution results for comparison, and port more tests from zetasql
What is the reason for having a new analysis test only?

dtenedor · 2023-03-15T20:51:19Z

@gengliangwang Sure, I was thinking about this too. We can reuse the same input SQL query files if we want, and just generate and test against different analyzer test output files. Let me update the PR to do that and I can ping the thread again.

dtenedor · 2023-03-15T20:52:35Z

@gengliangwang from past experience we will want to keep the query plans separate from the SQL results, otherwise the SQL results become hard to read. I will put the analyzer results in separate files.

gengliangwang · 2023-03-15T21:00:28Z

I will put the analyzer results in separate files.

Sounds great! Thanks for the work!

dtenedor · 2023-03-15T23:21:55Z

@gengliangwang alright I made this change, please look again when you are ready.

sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

dtenedor

Thanks @gengliangwang for your review!

sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala

sql/core/src/test/resources/sql-tests/analyzer-results/array.sql.out

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestHelper.scala

gengliangwang · 2023-03-16T21:43:07Z

LGTM except for minor comments. Thanks for the work!

dongjoon-hyun

Also, cc @sunchao

cloud-fan · 2023-03-17T06:23:03Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestHelper.scala

+    }
+
+  /** Returns all the files (not directories) in a directory, recursively. */
+  protected def listFilesRecursively(path: File): Seq[File] = {


I assume these 3 method are just copied from the existing code

Yes, I had moved these methods from their original place elsewhere in this file. I moved them back now to simplify the diff.

cloud-fan · 2023-03-17T06:36:42Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

+    def numSegments: Int
+  }
+
+  /** A single SQL query's output. */


Suggested change

/** A single SQL query's output. */

/** A single SQL query's execution output. */

cloud-fan · 2023-03-17T06:38:00Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

-      newLine.startsWith("--") && !newLine.startsWith("--QUERY-DELIMITER")
-    }
+  /** A test case. */
+  protected trait TestCase {


can we move it back to the original place? https://github.com/apache/spark/pull/40449/files#diff-212b35a074b1eeb518aff4c47130c7ab9cc23856ea6ea62f083af4452c637b9aL178

cloud-fan · 2023-03-17T06:40:49Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

@@ -338,6 +338,7 @@ class SQLQueryTestSuite extends QueryTest with SharedSparkSession with SQLHelper
      conf.trim -> value.substring(1).trim
    })

+    val regenerateGoldenFiles: Boolean = System.getenv("SPARK_GENERATE_GOLDEN_FILES") == "1"


why do we redefine the variable here?

removed this.

cloud-fan · 2023-03-17T06:41:47Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

+  protected def runSqlTestCase(
+      testCase: TestCase,
+      listTestCases: Seq[TestCase],
+      runQueries: (


why do we pass it as a parameter? We can directly invoke runQueries in runSqlTestCase

Good point, I removed this and just call runQueries directly now.

cloud-fan · 2023-03-17T06:43:03Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

+      val newTestCase =
      if (file.getAbsolutePath.startsWith(


Suggested change

val newTestCase =

if (file.getAbsolutePath.startsWith(

val newTestCase = if (file.getAbsolutePath.startsWith(

cloud-fan · 2023-03-17T06:43:32Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

+   * output of running a query.
+   */
+  def readGoldenFileAndCompareResults(
+    resultFile: String,


nit: 4 spaces indentation for parameters.

dtenedor

Thanks @cloud-fan for your review! Addressed comments.

dtenedor · 2023-03-18T19:03:21Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestHelper.scala

+    }
+
+  /** Returns all the files (not directories) in a directory, recursively. */
+  protected def listFilesRecursively(path: File): Seq[File] = {


Yes, I had moved these methods from their original place elsewhere in this file. I moved them back now to simplify the diff.

dtenedor · 2023-03-18T19:03:51Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

+  protected def runSqlTestCase(
+      testCase: TestCase,
+      listTestCases: Seq[TestCase],
+      runQueries: (


Good point, I removed this and just call runQueries directly now.

dtenedor · 2023-03-18T19:04:10Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

+   * output of running a query.
+   */
+  def readGoldenFileAndCompareResults(
+    resultFile: String,


dtenedor · 2023-03-18T19:04:33Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

+    def numSegments: Int
+  }
+
+  /** A single SQL query's output. */


dtenedor · 2023-03-18T19:05:11Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

+      val newTestCase =
      if (file.getAbsolutePath.startsWith(


dtenedor · 2023-03-18T19:05:26Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

@@ -338,6 +338,7 @@ class SQLQueryTestSuite extends QueryTest with SharedSparkSession with SQLHelper
      conf.trim -> value.substring(1).trim
    })

+    val regenerateGoldenFiles: Boolean = System.getenv("SPARK_GENERATE_GOLDEN_FILES") == "1"


removed this.

dtenedor · 2023-03-18T19:06:12Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

-      newLine.startsWith("--") && !newLine.startsWith("--QUERY-DELIMITER")
-    }
+  /** A test case. */
+  protected trait TestCase {


cloud-fan · 2023-03-20T11:49:16Z

thanks, merging to master!

LuciferYang · 2023-03-20T15:14:39Z

Seems GA break after this one:

cloud-fan · 2023-03-20T15:19:37Z

I'm AFK at this time. @gengliangwang can you help to revert it if @dtenedor can't fix the failure soon?

LuciferYang · 2023-03-20T15:25:22Z

Seems would be ok to regenerate the golden files

LuciferYang · 2023-03-20T15:32:38Z

#40492

dtenedor · 2023-03-20T15:40:49Z

Looks like some extra tests got added just as this was getting merged! Thanks @LuciferYang for this fix 👍

…end` ### What changes were proposed in this pull request? This pr re-generates golden files for `array_prepend` functions. It seems that the newly added case in #38947 is missing from the golden files due to lack of rebase when merging #40449. ### Why are the changes needed? Re-generates golden files for `array_prepend` functions to Pass GitHub Actions ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? - Pass GitHub Actions - Manually checked with Scala 2.13 Closes #40492 from LuciferYang/SPARK-42791-FOLLOWUP. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

… for all input files ### What changes were proposed in this pull request? This PR enables the new golden file test framework for analysis for all input files. Background: * In #40449 we added the ability to exercise the analyzer on the SQL queries in existing golden files in the `sql/core/src/test/resources/sql-tests/inputs` directory, writing separate output test files in the new `sql/core/src/test/resources/sql-tests/analyzer-results` directory in additional to the original output directory for full end-to-end query execution results. * That PR also added an allowlist of input files to include in this new dual-run mode. * In this PR, we remove that allowlist exercise the new dual-run mode for all the input files. We also extend the analyzer testing to support separate test cases in ANSI-mode, TimestampNTZ, and UDFs. ### Why are the changes needed? This improves test coverage and helps prevent against accidental regressions in the future as we edit the code. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? This PR adds testing only. Closes #40496 from dtenedor/add-all-test-files. Authored-by: Daniel Tenedorio <daniel.tenedorio@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

commit

c33f74c

commit commit commit commit

github-actions bot added the SQL label Mar 15, 2023

dtenedor added 3 commits March 15, 2023 11:48

commit

5392f9e

clean up output

994c9e6

clean up output

e68faa6

update

a677845

dtenedor added 3 commits March 15, 2023 15:18

commit

4efe032

commit

9b908a9

respond to code review comments

d50b92c

fix compile

dd2c28e

gengliangwang reviewed Mar 16, 2023

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala Outdated Show resolved Hide resolved

fix build

dc994ee

gengliangwang reviewed Mar 16, 2023

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala Show resolved Hide resolved

gengliangwang reviewed Mar 16, 2023

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala Outdated Show resolved Hide resolved

dtenedor commented Mar 16, 2023

View reviewed changes

dtenedor added 2 commits March 16, 2023 14:23

respond to code review comments

2009b91

add missing file

e0182f9

gengliangwang reviewed Mar 16, 2023

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala Outdated Show resolved Hide resolved

gengliangwang reviewed Mar 16, 2023

View reviewed changes

sql/core/src/test/resources/sql-tests/analyzer-results/array.sql.out Outdated Show resolved Hide resolved

respond to code review comments

3f49ee7

dtenedor requested a review from gengliangwang March 16, 2023 21:42

gengliangwang reviewed Mar 16, 2023

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestHelper.scala Outdated Show resolved Hide resolved

respond to code review comments

26e7a31

respond to code review comments

87e65fd

gengliangwang approved these changes Mar 16, 2023

View reviewed changes

respond to code review comments

35e1bd6

dongjoon-hyun reviewed Mar 17, 2023

View reviewed changes

cloud-fan reviewed Mar 17, 2023

View reviewed changes

dtenedor commented Mar 18, 2023

View reviewed changes

dtenedor added 2 commits March 18, 2023 12:07

respond to code review comments

492069b

remove unused import

1c8a0fa

cloud-fan approved these changes Mar 20, 2023

View reviewed changes

cloud-fan closed this in 6747bee Mar 20, 2023

LuciferYang mentioned this pull request Mar 20, 2023

[SPARK-42791][SQL][FOLLOWUP] Re-generate golden files for array_prepend #40492

Closed

dtenedor mentioned this pull request Mar 20, 2023

[SPARK-42874][SQL] Enable new golden file test framework for analysis for all input files #40496

Closed

	/** A single SQL query's output. */
	/** A single SQL query's execution output. */

	val newTestCase =
	if (file.getAbsolutePath.startsWith(
	val newTestCase = if (file.getAbsolutePath.startsWith(

[SPARK-42791][SQL] Create a new golden file test framework for analysis #40449

[SPARK-42791][SQL] Create a new golden file test framework for analysis #40449

Conversation

dtenedor commented Mar 15, 2023 • edited Loading

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?

dtenedor commented Mar 15, 2023

gengliangwang commented Mar 15, 2023

dtenedor commented Mar 15, 2023

dtenedor commented Mar 15, 2023

gengliangwang commented Mar 15, 2023

dtenedor commented Mar 15, 2023

dtenedor left a comment

Choose a reason for hiding this comment

gengliangwang commented Mar 16, 2023

dongjoon-hyun left a 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

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

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

Choose a reason for hiding this comment

dtenedor left a 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

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

cloud-fan commented Mar 20, 2023

LuciferYang commented Mar 20, 2023

cloud-fan commented Mar 20, 2023

LuciferYang commented Mar 20, 2023

LuciferYang commented Mar 20, 2023

dtenedor commented Mar 20, 2023

dtenedor commented Mar 15, 2023 •

edited

Loading