[SPARK-21832][TEST] Merge SQLBuilderTest into ExpressionSQLBuilderSuite #19044
Conversation
After SPARK-19025, there is no need to keep SQLBuilderTest. ExpressionSQLBuilderSuite is the only place to use it. This PR aims to remove SQLBuilderTest.
| } | ||
| } | ||
|
|
||
| import org.apache.spark.sql.catalyst.dsl.expressions._ |
There was a problem hiding this comment.
Thank you for review, @gatorsmile . I thought we can remove SQLBuilderTest because we will use SQL generation on expression level only.
There was a problem hiding this comment.
Oh, I see. There is overloading issue on ===.
Line 31: === is string comparision.
assert(actualSQL === expectedSQL)
The import is used to make predicate with === in the below test cases.
import org.apache.spark.sql.catalyst.dsl.expressions._
There was a problem hiding this comment.
Then, we should move it to the test case it needs such an import. Also add a comment there.
There was a problem hiding this comment.
Thank you. I'll update like that.
|
Test build #81101 has finished for PR 19044 at commit
|
| protected def checkSQL(e: Expression, expectedSQL: String): Unit = { | ||
| val actualSQL = e.sql | ||
| try { | ||
| assert(actualSQL === expectedSQL) |
There was a problem hiding this comment.
I thought just one test case required that import.
Since most cases require it, you just need to change === => ==. Then, you can move the import to the beginning of this file.
There was a problem hiding this comment.
I see. Yep. That's better.
There was a problem hiding this comment.
The PR is updated. Thanks!
|
Test build #81106 has finished for PR 19044 at commit
|
|
LGTM pending Jenkins |
|
Thank you for review, @gatorsmile ! |
|
Test build #81107 has finished for PR 19044 at commit
|
|
Retest this please. |
|
Test build #81113 has finished for PR 19044 at commit
|
|
Thanks! Merging to master. |
|
Thank you for review and merging, @gatorsmile . |
What changes were proposed in this pull request?
After SPARK-19025, there is no need to keep SQLBuilderTest.
ExpressionSQLBuilderSuite is the only place to use it.
This PR aims to remove SQLBuilderTest.
How was this patch tested?
Pass the updated
ExpressionSQLBuilderSuite.