-
Notifications
You must be signed in to change notification settings - Fork 28.4k
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.
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
[SPARK-16866][SQL] Infrastructure for file-based SQL end-to-end tests #14472
Changes from 3 commits
ba9b678
9b360da
a1e1b57
2352d6f
34b6704
0359756
5eb01fe
26c7771
7497742
14f4959
288b699
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -99,4 +99,5 @@ spark-deps-.* | |
.*tsv | ||
org.apache.spark.scheduler.ExternalClusterManager | ||
.*\.sql | ||
.*\.sql\.xml | ||
.Rbuildignore |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
-- This is a query file that has been blacklisted. | ||
-- It includes a query that should crash Spark. | ||
-- If the test case is run, the whole suite would fail. | ||
some random not working query that should crash Spark. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
-- Verifies how we parse numbers | ||
|
||
-- parse as ints | ||
select 1, -1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if the query need to read data from some pre-defined tables? do you have a plan for this feature? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spark SQL does not yet have a great strategy for this. I think we can have a small set of predefined tables, and then create tables on the fly using "create table x as select 1, 2, 3 union all select 2, 3, 4". We will likely need to iterate on this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can also use this: #14539 |
||
|
||
-- parse as longs | ||
select 2147483648, -2147483649; | ||
|
||
-- parse as decimals | ||
select 9223372036854775808, -9223372036854775809; | ||
|
||
-- various floating point formats | ||
select 0.3, -0.8, .5, -.18; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
-- Automatically generated by org.apache.spark.sql.SQLQueryTestSuite | ||
-- Number of queries: 4 | ||
|
||
|
||
-- !query 0 | ||
select 1, -1 | ||
-- !query 0 schema | ||
int, int | ||
-- !query 0 output | ||
+---+----+ | ||
| 1|(-1)| | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Putting the result of
We need a way to encode output data into string and parse it back. A simple way is to use |
||
+---+----+ | ||
| 1| -1| | ||
+---+----+ | ||
|
||
|
||
-- !query 1 | ||
select 2147483648, -2147483649 | ||
-- !query 1 schema | ||
bigint, bigint | ||
-- !query 1 output | ||
+----------+-------------+ | ||
|2147483648|(-2147483649)| | ||
+----------+-------------+ | ||
|2147483648| -2147483649| | ||
+----------+-------------+ | ||
|
||
|
||
-- !query 2 | ||
select 9223372036854775808, -9223372036854775809 | ||
-- !query 2 schema | ||
decimal(19,0), decimal(19,0) | ||
-- !query 2 output | ||
+-------------------+----------------------+ | ||
|9223372036854775808|(-9223372036854775809)| | ||
+-------------------+----------------------+ | ||
|9223372036854775808| -9223372036854775809| | ||
+-------------------+----------------------+ | ||
|
||
|
||
-- !query 3 | ||
select 0.3, -0.8, .5, -.18 | ||
-- !query 3 schema | ||
decimal(1,1), decimal(1,1), decimal(1,1), decimal(2,2) | ||
-- !query 3 output | ||
+---+------+---+-------+ | ||
|0.3|(-0.8)|0.5|(-0.18)| | ||
+---+------+---+-------+ | ||
|0.3| -0.8|0.5| -0.18| | ||
+---+------+---+-------+ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,203 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* contributor license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright ownership. | ||
* The ASF licenses this file to You under the Apache License, Version 2.0 | ||
* (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package org.apache.spark.sql | ||
|
||
import java.io.File | ||
import java.util.{Locale, TimeZone} | ||
|
||
import org.apache.spark.sql.catalyst.rules.RuleExecutor | ||
import org.apache.spark.sql.catalyst.util.{fileToString, stringToFile} | ||
import org.apache.spark.sql.test.SharedSQLContext | ||
|
||
/** | ||
* End-to-end test cases for SQL queries. | ||
* | ||
* Each case is loaded from a file in "spark/sql/core/src/test/resources/sql-tests/inputs". | ||
* Each case has a golden result file in "spark/sql/core/src/test/resources/sql-tests/results". | ||
* | ||
* To re-generate golden files, run: | ||
* {{{ | ||
* SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "sql/test-only *SQLQueryTestSuite" | ||
* }}} | ||
* | ||
* The format for input files is simple: | ||
* 1. A list of SQL queries separated by semicolon. | ||
* 2. Lines starting with -- are treated as comments and ignored. | ||
* | ||
* For example: | ||
* {{{ | ||
* -- this is a comment | ||
* select 1, -1; | ||
* select current_date; | ||
* }}} | ||
* | ||
* The format for golden result files look roughly like: | ||
* {{{ | ||
* -- some header information | ||
* | ||
* -- !query 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why putting a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to make it less likely that somebody actually write "-- query" in a comment. |
||
* select 1, -1 | ||
* -- !query 0 schema | ||
* int, int | ||
* -- !query 0 output | ||
* +---+----+ | ||
* | 1|(-1)| | ||
* +---+----+ | ||
* | 1| -1| | ||
* +---+----+ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. update this comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
* | ||
* -- !query 1 | ||
* ... | ||
* }}} | ||
*/ | ||
class SQLQueryTestSuite extends QueryTest with SharedSQLContext { | ||
|
||
private val regenerateGoldenFiles: Boolean = System.getenv("SPARK_GENERATE_GOLDEN_FILES") == "1" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to have another program to generate the golden files, e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why putting the two into separate programs? This approach seems easy enough and was the one suggested by @rxin in LogicalPlanToSQLSuite. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically majority of the code are the same. |
||
|
||
private val inputFilePath = "src/test/resources/sql-tests/inputs/" | ||
private val goldenFilePath = "src/test/resources/sql-tests/results/" | ||
|
||
/** List of test cases to ignore, in lower cases. */ | ||
private val blackList = Set( | ||
"blacklist.sql" // Do NOT remove this one. It is here to test the blacklist functionality. | ||
) | ||
|
||
// Create all the test cases. | ||
listTestCases().foreach(createScalaTestCase) | ||
|
||
/** A test case. */ | ||
private case class TestCase(name: String, inputFile: String, resultFile: String) | ||
|
||
/** A single SQL query's output. */ | ||
private case class QueryOutput(sql: String, schema: String, output: String) { | ||
def toString(queryIndex: Int): String = { | ||
// We are explicitly not using multi-line string due to stripMargin removing "|" in output. | ||
s"-- !query $queryIndex\n" + | ||
sql + "\n" + | ||
s"-- !query $queryIndex schema\n" + | ||
schema + "\n" + | ||
s"-- !query $queryIndex output\n" + | ||
output | ||
} | ||
} | ||
|
||
private def createScalaTestCase(testCase: TestCase): Unit = { | ||
if (blackList.contains(testCase.name.toLowerCase)) { | ||
// Create a test case to ignore this case. | ||
ignore(testCase.name) { /* Do nothing */ } | ||
} else { | ||
// Create a test case to run this case. | ||
test(testCase.name) { runTest(testCase) } | ||
} | ||
} | ||
|
||
/** Run a test case. */ | ||
private def runTest(testCase: TestCase): Unit = { | ||
val input = fileToString(new File(testCase.inputFile)) | ||
|
||
// List of SQL queries to run | ||
val queries: Seq[String] = { | ||
// val cleaned = input.split("\n").filterNot(_.matches("--.*(?<=[^\\\\]);")).mkString("\n") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove this line? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
val cleaned = input.split("\n").filterNot(_.startsWith("--")).mkString("\n") | ||
// note: this is not a robust way to split queries using semicolon, but works for now. | ||
cleaned.split("(?<=[^\\\\]);").map(_.trim).filterNot(q => q == "").toSeq | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't it strictly less clear? It is more obvious that this is a string this way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed it to filter(_ != "") |
||
} | ||
|
||
// Run the SQL queries preparing them for comparison. | ||
val outputs: Seq[QueryOutput] = queries.map { sql => | ||
val df = spark.sql(sql) | ||
// We might need to do some query canonicalization in the future. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not yet doing any sorting of outputs here. Not sure if it is necessary yet. |
||
QueryOutput( | ||
sql = sql, | ||
schema = df.schema.map(_.dataType.simpleString).mkString(", "), | ||
output = df.showString(_numRows = 10000, truncate = 10000).trim) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using showString might not be the most friendly when there is a mismatch and the output is huge, but should work very well with smaller outputs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
|
||
if (regenerateGoldenFiles) { | ||
// Again, we are explicitly not using multi-line string due to stripMargin removing "|". | ||
val goldenOutput = { | ||
s"-- Automatically generated by ${getClass.getName}\n" + | ||
s"-- Number of queries: ${outputs.size}\n\n\n" + | ||
outputs.zipWithIndex.map{case (qr, i) => qr.toString(i)}.mkString("\n\n\n") + "\n" | ||
} | ||
stringToFile(new File(testCase.resultFile), goldenOutput) | ||
} | ||
|
||
// Read back the golden file. | ||
val expectedOutputs: Seq[QueryOutput] = { | ||
val goldenOutput = fileToString(new File(testCase.resultFile)) | ||
val segments = goldenOutput.split("-- !query.+\n") | ||
assert(segments.size == outputs.size * 3 + 1) // each query has 3 segments, plus the header | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that a problem that some asserts are never hit? Some logic might change and then one of the asserts can fail. I prefer to be more conservative for asserts. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But this assert doesn't match the logic in this branch. What we do here is skipping the first segment, and grouping the rest segments by 3, each group means a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But then why would % 3 be better? Are you arguing for a better message when it fails? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get "this assert doesn't match the logic in this branch". There is no logic that dictates we cannot verify the number of blocks here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if Anyway it's bad to have dead code, if we wanna keep this assert, we should remove https://github.com/apache/spark/pull/14472/files#diff-432455394ca50800d5de508861984ca5R164 and move its error message here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will add a better error message, but I'm afraid I disagree with you on removing the other assert. It is not dead code because it is exercised at runtime. They are making different assumptions at different places in the code. We could change the way we arrange blocks in the future and then the other assert would be useful. Anyway I am not sure why you are nitpicking on this. It seems very minor and we are simply wasting time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically asserts are used as defensive guards against program errors. By your definition almost all asserts are "dead code". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I get where you are coming from. You think the assert as something to verify correctness for a test case (in the ScalaTest sense). I was using assert as a defensive guard to catch error (as in basic invariants that shouldn't have been violated for this tiny block). |
||
Seq.tabulate(outputs.size) { i => | ||
QueryOutput( | ||
sql = segments(i * 3 + 1).trim, | ||
schema = segments(i * 3 + 2).trim, | ||
output = segments(i * 3 + 3).trim | ||
) | ||
} | ||
} | ||
|
||
// Compare results. | ||
assertResult(expectedOutputs.size, s"Number of queries should be ${expectedOutputs.size}") { | ||
outputs.size | ||
} | ||
|
||
outputs.zip(expectedOutputs).zipWithIndex.foreach { case ((output, expected), i) => | ||
assertResult(expected.sql, s"SQL query should match for query #$i") { output.sql } | ||
assertResult(expected.schema, s"Schema should match for query #$i") { output.schema } | ||
assertResult(expected.output, s"Result should match for query #$i") { output.output } | ||
} | ||
} | ||
|
||
private def listTestCases(): Seq[TestCase] = { | ||
listFilesRecursively(new File(inputFilePath)).map { file => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this might not work for Maven - I will look into this later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. now it should work for maven |
||
val resultFile = file.getAbsolutePath.replace(inputFilePath, goldenFilePath) + ".out" | ||
TestCase(file.getName, file.getAbsolutePath, resultFile) | ||
} | ||
} | ||
|
||
/** Returns all the files (not directories) in a directory, recursively. */ | ||
private def listFilesRecursively(path: File): Seq[File] = { | ||
val (dirs, files) = path.listFiles().partition(_.isDirectory) | ||
files ++ dirs.flatMap(listFilesRecursively) | ||
} | ||
|
||
private val originalTimeZone = TimeZone.getDefault | ||
private val originalLocale = Locale.getDefault | ||
|
||
override def beforeAll(): Unit = { | ||
super.beforeAll() | ||
// Timezone is fixed to America/Los_Angeles for those timezone sensitive tests (timestamp_*) | ||
TimeZone.setDefault(TimeZone.getTimeZone("America/Los_Angeles")) | ||
// Add Locale setting | ||
Locale.setDefault(Locale.US) | ||
RuleExecutor.resetTime() | ||
} | ||
|
||
override def afterAll(): Unit = { | ||
try { | ||
TimeZone.setDefault(originalTimeZone) | ||
Locale.setDefault(originalLocale) | ||
|
||
// For debugging dump some statistics about how much time was spent in various optimizer rules | ||
logWarning(RuleExecutor.dumpTimeSpent()) | ||
} finally { | ||
super.afterAll() | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add a rule for
.out
?