Skip to content

[SPARK-29873][SQL][Test][FOLLOWUP] set operations should not escape when regen golden file with --SET --import both specified #26557

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
yaooqinn wants to merge 6 commits into from
Closed

[SPARK-29873][SQL][Test][FOLLOWUP] set operations should not escape when regen golden file with --SET --import both specified #26557

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a854316
[SPARK-29873][SQL] set operations should not escape when regen golden…
yaooqinn Nov 16, 2019
c791b36
rm changes in literals.sql
yaooqinn Nov 16, 2019
8fb683e
rm tests
yaooqinn Nov 16, 2019
79d27c6
Merge branch 'master' into SPARK-29873
yaooqinn Nov 18, 2019
538a44f
comments, to upper case
yaooqinn Nov 18, 2019
3192e15
comment
yaooqinn Nov 18, 2019
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
2 changes: 1 addition & 1 deletion sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ class SQLQueryTestSuite extends QueryTest with SharedSparkSession {

// 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) {
if ((regenerateGoldenFiles && importedTestCaseName.isEmpty) || !isTestWithConfigSets) {
Copy link
Contributor

@cloud-fan cloud-fan Nov 18, 2019 •
edited
Loading

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

@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.

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

@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.

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

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.

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

Copy link
Member Author

@yaooqinn yaooqinn Nov 18, 2019

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

@wangyum wangyum Nov 18, 2019

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

@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.

do you mean setting config is expensive?

Copy link
Member

@wangyum wangyum Nov 18, 2019

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:

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

Lines 287 to 298 in ec5d698

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

@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 that's a problem. We should think about how to reduce test time.

Copy link
Member

@maropu maropu Nov 20, 2019

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 = {
Expand Down