[SPARK-30141][SQL][TESTS] Fix QueryTest.checkAnswer usage #26768
Conversation
|
Test build #114904 has finished for PR 26768 at commit
|
| Row("5", 2) :: Row("6", 2) :: Row("7", 1) :: Row("8", 1) :: Row("9", 1) :: Nil) match { | ||
| case Some(errorMessage) => fail(errorMessage) | ||
| case None => | ||
| } |
There was a problem hiding this comment.
This seems to fail like the following at schema comparison.
!== Correct Answer - 9 == == Spark Answer - 9 ==
!struct<> struct<value:int,count(1):bigint>
[1,1] [1,1]
[2,1] [2,1]
[3,2] [3,2]
[4,2] [4,2]
[5,2] [5,2]
[6,2] [6,2]
[7,1] [7,1]
[8,1] [8,1]
[9,1] [9,1]
There was a problem hiding this comment.
This is because expectedAnswer's data type do not match. 61eacc9 fix this issue.
There was a problem hiding this comment.
Before this PR. it will not fail.
| countAcc.setValue(0) | ||
| QueryTest.checkAnswer( | ||
| df, Seq(expectedResult), checkToRDD = false /* avoid duplicate exec */) | ||
| df, Seq(expectedResult), checkToRDD = false /* avoid duplicate exec */) match { |
There was a problem hiding this comment.
Why not adding a new method in QueryTest object to let it call fail instead? The pattern is simply duplicated across changes, and even QueryTest.checkAnswer (protected method), which warrants enough reason to have another method.
e.g. checkAnswer(df: DataFrame, expectedAnswer: java.util.List[Row]): String
There was a problem hiding this comment.
Otherwise, maybe simply call checkAnswer (without QueryTest) - other places in this suite already called like this. Same applies for other suites as well.
|
Test build #114923 has finished for PR 26768 at commit
|
| val caught = intercept[SparkException] { | ||
| withSQLConf(SOURCES_BINARY_FILE_MAX_LENGTH.key -> (content.length - 1).toString) { | ||
| QueryTest.checkAnswer(readContent(), expected) | ||
| QueryTest.checkAnswer(readContent(), expected) match { |
There was a problem hiding this comment.
Can we ..
- make
QueryTest.checkAnswerprivate so that it can only be called in its companion classQueryTest - and avoid to call
checkAnswerin theQueryTestobject in the tests
?
There was a problem hiding this comment.
I think @HyukjinKwon's suggestion would be better for future modification in point of preventing the cause, if there's no actual usage of QueryTest.checkAnswer from outside of QueryTest. (I guess it might have been misused like this PR fixes...)
5 participants
What changes were proposed in this pull request?
This PR fixes the usage of
QueryTest.checkAnswer. It should fail if the results do not match.Why are the changes needed?
Fix test bug.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
N/A