-
Notifications
You must be signed in to change notification settings - Fork 28k
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-40936][SQL][TESTS] Refactor AnalysisTest#assertAnalysisErrorClass
by reusing the SparkFunSuite#checkError
#38413
Conversation
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisTest.scala
Show resolved
Hide resolved
cc @MaxGekk @HyukjinKwon @dongjoon-hyun Think again, does this refactor look more simple? |
I wonder why do we need Let's consider to invoke |
Sounds good, let me try. Set this to draft first and will ping you when it can be reviewed @MaxGekk |
AnalysisTest#assertAnalysisErrorClass
method AnalysisTest#assertAnalysisErrorClass
method
caseSensitive = true, | ||
line = -1, | ||
pos = -1) | ||
Array(ExpectedContext("y", 46, 46)) |
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.
spark/core/src/test/scala/org/apache/spark/SparkFunSuite.scala
Lines 319 to 332 in cf086b1
val actualQueryContext = exception.getQueryContext() | |
assert(actualQueryContext.length === queryContext.length, "Invalid length of the query context") | |
actualQueryContext.zip(queryContext).foreach { case (actual, expected) => | |
assert(actual.objectType() === expected.objectType(), | |
"Invalid objectType of a query context Actual:" + actual.toString) | |
assert(actual.objectName() === expected.objectName(), | |
"Invalid objectName of a query context. Actual:" + actual.toString) | |
assert(actual.startIndex() === expected.startIndex(), | |
"Invalid startIndex of a query context. Actual:" + actual.toString) | |
assert(actual.stopIndex() === expected.stopIndex(), | |
"Invalid stopIndex of a query context. Actual:" + actual.toString) | |
assert(actual.fragment() === expected.fragment(), | |
"Invalid fragment of a query context. Actual:" + actual.toString) | |
} |
This change is due to the checkError
method will perform a forced check when actualQueryContext
is not empty. If we can to relax some check conditions, can add a precondition queryContext.nonEmpty
for the queryContext
check.
AnalysisTest#assertAnalysisErrorClass
method AnalysisTest#assertAnalysisErrorClass
by reusing the SparkFunSuite#checkError
AnalysisTest#assertAnalysisErrorClass
by reusing the SparkFunSuite#checkError
AnalysisTest#assertAnalysisErrorClass
by reusing the SparkFunSuite#checkError
AnalysisTest#assertAnalysisErrorClass
by reusing the SparkFunSuite#checkError
AnalysisTest#assertAnalysisErrorClass
method
Refactor
Please review this if you have time, thanks ~ |
AnalysisTest#assertAnalysisErrorClass
method AnalysisTest#assertAnalysisErrorClass
by reusing the SparkFunSuite#checkError
Waiting for Ci. |
GA passed |
+1, LGTM. Merging to master. |
…lass` by reusing the `SparkFunSuite#checkError` ### What changes were proposed in this pull request? This pr aims to refactor `AnalysisTest#assertAnalysisErrorClass` method by reusing the `checkError` method in `SparkFunSuite`. On the other hand, the signature of `AnalysisTest#assertAnalysisErrorClass` method is changed from ``` protected def assertAnalysisErrorClass( inputPlan: LogicalPlan, expectedErrorClass: String, expectedMessageParameters: Map[String, String], caseSensitive: Boolean = true, line: Int = -1, pos: Int = -1): Unit ``` to ``` protected def assertAnalysisErrorClass( inputPlan: LogicalPlan, expectedErrorClass: String, expectedMessageParameters: Map[String, String], queryContext: Array[QueryContext] = Array.empty, caseSensitive: Boolean = true): Unit ``` Then when we need to use `queryContext` instead of `line + pos` for assertion ### Why are the changes needed? `assertAnalysisErrorClass` and `checkError` does the same work. ### Does this PR introduce _any_ user-facing change? No, just for test ### How was this patch tested? - Pass GitHub Actions Closes apache#38413 from LuciferYang/simplify-assertAnalysisErrorClass. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>
What changes were proposed in this pull request?
This pr aims to refactor
AnalysisTest#assertAnalysisErrorClass
method by reusing thecheckError
method inSparkFunSuite
.On the other hand, the signature of
AnalysisTest#assertAnalysisErrorClass
method is changed fromto
Then when we need to use
queryContext
instead ofline + pos
for assertionWhy are the changes needed?
assertAnalysisErrorClass
andcheckError
does the same work.Does this PR introduce any user-facing change?
No, just for test
How was this patch tested?