Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1 +1 @@
--import higher-order-functions.sql
--IMPORT higher-order-functions.sql
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
--import interval.sql
--IMPORT interval.sql

-- the `interval` keyword can be omitted with ansi mode
select 1 year 2 days;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
--- malformed interval literal with ansi mode
--import literals.sql
--IMPORT literals.sql
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
-- tests for interval output style with iso_8601 format
--SET spark.sql.intervalOutputStyle = ISO_8601
--import interval-display.sql
--IMPORT interval-display.sql
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
-- tests for interval output style with sql standard format
--SET spark.sql.intervalOutputStyle = SQL_STANDARD
--import interval-display.sql
--IMPORT interval-display.sql
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ import org.apache.spark.tags.ExtendedSQLTest
* 1. A list of SQL queries separated by semicolon.
* 2. Lines starting with -- are treated as comments and ignored.
* 3. Lines starting with --SET are used to run the file with the following set of configs.
* 4. Lines starting with --import are used to load queries from another test file.
* 4. Lines starting with --IMPORT are used to load queries from another test file.
*
* For example:
* {{{
Expand Down Expand Up @@ -265,9 +265,9 @@ class SQLQueryTestSuite extends QueryTest with SharedSparkSession {

val (comments, code) = input.split("\n").partition(_.trim.startsWith("--"))

// If `--import` found, load code from another test case file, then insert them
// If `--IMPORT` found, load code from another test case file, then insert them
// into the head in this test.
val importedTestCaseName = comments.filter(_.startsWith("--import ")).map(_.substring(9))
val importedTestCaseName = comments.filter(_.startsWith("--IMPORT ")).map(_.substring(9))
val importedCode = importedTestCaseName.flatMap { testCaseName =>
listTestCases.find(_.name == testCaseName).map { testCase =>
val input = fileToString(new File(testCase.inputFile))
Expand All @@ -283,13 +283,17 @@ class SQLQueryTestSuite extends QueryTest with SharedSparkSession {
// Fix misplacement when comment is at the end of the query.
.map(_.split("\n").filterNot(_.startsWith("--")).mkString("\n")).map(_.trim).filter(_ != "")

// When we are regenerating the golden files, we don't need to set any config as they
// all need to return the same result
if (regenerateGoldenFiles || !isTestWithConfigSets) {
// When we are regenerating the golden files for test cases without '--IMPORT' specified, or
// running test cases against [[ThriftServerQueryTestSuite], we don't need to set any config as
// they all need to return the same result.
// When we use '--SET' and '--IMPORT' together for those import queries, we want to run the
// same queries from the original file but with different settings and save the answers. So the
// `--SET` will be respected in this case.
if ((regenerateGoldenFiles && importedTestCaseName.isEmpty) || !isTestWithConfigSets) {
Copy link
Contributor

@cloud-fan cloud-fan Nov 18, 2019

Choose a reason for hiding this comment

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

ah seems we have 2 different kinds of settings now:

  1. settings that shouldn't change result. We want to generate the golden files without the settings and run tests with settings.
  2. setting that can change result.

The second one is pretty useful with --import. We want to run the same queries with different settings and save the answers.

Shall we have different directives for them? @maropu

Copy link
Contributor

Choose a reason for hiding this comment

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

After a second thought, we only expect the --SET to change behavior if we import queries. So the change here LGTM.

@yaooqinn can you update the comment to explain it?

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW now --SET is upper case and --import is lower case. Seems better to always use upper case.

Copy link
Contributor

Choose a reason for hiding this comment

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

also cc @wangyum , why do we need isTestWithConfigSets for thriftserver tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I will update the comments and change case for --import

Copy link
Member

Choose a reason for hiding this comment

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

@cloud-fan To reduce the test time. The mvn test often timed out at that time: https://amplab.cs.berkeley.edu/jenkins/job/spark-master-test-maven-hadoop-2.7/

Copy link
Contributor

Choose a reason for hiding this comment

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

do you mean setting config is expensive?

Copy link
Member

Choose a reason for hiding this comment

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

It will run configSets times:

configSets.foreach { configSet =>
try {
runQueries(queries, testCase, Some(configSet))
} catch {
case e: Throwable =>
val configs = configSet.map {
case (k, v) => s"$k=$v"
}
logError(s"Error using configs: ${configs.mkString(",")}")
throw e
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

ah that's a problem. We should think about how to reduce test time.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I'm late. The change looks reasonable...

runQueries(queries, testCase, None)
} else {
val configSets = {
val configLines = comments.filter(_.startsWith("--SET")).map(_.substring(5))
val configLines = comments.filter(_.startsWith("--SET ")).map(_.substring(6))
val configs = configLines.map(_.split(",").map { confAndValue =>
val (conf, value) = confAndValue.span(_ != '=')
conf.trim -> value.substring(1).trim
Expand Down