-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-32762][SQL][TEST] Enhance the verification of ExpressionsSchemaSuite to sql-expression-schema.md #29608
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
Changes from 3 commits
6d36603
5c8e29e
bf0ceea
cbb0fcd
85299b1
5a574c3
6e6489b
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 |
|---|---|---|
|
|
@@ -152,23 +152,37 @@ class ExpressionsSchemaSuite extends QueryTest with SharedSparkSession { | |
|
|
||
| val outputSize = outputs.size | ||
| val headerSize = header.size | ||
| val expectedOutputs: Seq[QueryOutput] = { | ||
| val (expectedMissingExamples, expectedOutputs) = { | ||
| val expectedGoldenOutput = fileToString(resultFile) | ||
| val lines = expectedGoldenOutput.split("\n") | ||
| val expectedSize = lines.size | ||
|
|
||
| assert(expectedSize == outputSize + headerSize, | ||
| s"Expected $expectedSize blocks in result file but got " + | ||
| s"${outputSize + headerSize}. Try regenerate the result files.") | ||
| s"${outputSize + headerSize}. Try regenerating the result files.") | ||
|
|
||
| Seq.tabulate(outputSize) { i => | ||
| val numberOfQueries = lines(2).split(":")(1).trim.toInt | ||
| val numberOfMissingExample = lines(3).split(":")(1).trim.toInt | ||
| val missingExamples = lines(4).split(":")(1).trim.split(",") | ||
|
Contributor
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.
Contributor
Author
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 expectedOutputs = Seq.tabulate(outputSize) { i => | ||
| val segments = lines(i + headerSize).split('|') | ||
| QueryOutput( | ||
| className = segments(1).trim, | ||
| funcName = segments(2).trim, | ||
| sql = segments(3).trim, | ||
| schema = segments(4).trim) | ||
| } | ||
|
|
||
| // Ensure consistency of the result file. | ||
| assert(numberOfQueries == expectedOutputs.size, | ||
|
Contributor
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 should put these assert on line 163
Contributor
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.
Contributor
Author
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. @beliefer This assert place here to verify the consistency of the file contents to avoid inconsistency caused by manual modification, and I think line 189
Contributor
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. You can try to mock a new function with comments and test this suite.
Contributor
Author
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.
You are right, Spark-24884 already trigger this problem cause by incomplete manual modification. With the new function add scene, I think the right way is run this case with
Contributor
Author
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. Therefore, if the file is manually modified instead of automatically generated, I think the assertion failure caused by incorrect modification should be expected. Do we need to tell users clearly with
Contributor
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 got it. Thanks! Could you put these assert on line 163, so that it looks clear
Contributor
Author
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. Address 6e6489b reorder the assertions. |
||
| s"outputs size: ${expectedOutputs.size} not same as numberOfQueries: $numberOfQueries " + | ||
| "record in result file. Try regenerating the result files.") | ||
| assert(numberOfMissingExample == missingExamples.size, | ||
| s"missing examples size: ${missingExamples.size} not same as " + | ||
| s"numberOfMissingExample: $numberOfMissingExample " + | ||
| "record in result file. Try regenerating the result files.") | ||
|
|
||
| (missingExamples, expectedOutputs) | ||
|
Contributor
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. ditto
Contributor
Author
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 ~ |
||
| } | ||
|
|
||
| // Compare results. | ||
|
|
@@ -179,5 +193,13 @@ class ExpressionsSchemaSuite extends QueryTest with SharedSparkSession { | |
| assert(expected.sql == output.sql, "SQL query did not match") | ||
| assert(expected.schema == output.schema, s"Schema did not match for query ${expected.sql}") | ||
| } | ||
|
|
||
| // Compare expressions missing examples | ||
| assert(expectedMissingExamples.length == missingExamples.size, | ||
| "The number of missing examples not equals the number of expected missing examples.") | ||
|
|
||
| missingExamples.zip(expectedMissingExamples).foreach { case (output, expected) => | ||
| assert(expected == output, "Missing example expression not match") | ||
| } | ||
| } | ||
| } | ||
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.
nit:
Example->ExamplesThere 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.
done ~