You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/10/27 12:44:06 UTC
[GitHub] [spark] LuciferYang opened a new pull request, #38413: [SPARK-40936][SQL][TESTS] Remove outer conditions to simplify `AnalysisTest#assertAnalysisErrorClass` method
LuciferYang opened a new pull request, #38413:
URL: https://github.com/apache/spark/pull/38413
### What changes were proposed in this pull request?
This pr try to simplify `AnalysisTest#assertAnalysisErrorClass` method in the following way:
- Remove the outer conditions: `e.getErrorClass != expectedErrorClass || e.messageParameters != expectedMessageParameters || (line >= 0 && e.line.getOrElse(-1) != line) || (pos >= 0 && e.startPosition.getOrElse(-1) != pos)`
- Use `val` StringBuilder instead of `var` String and fail when `StringBuilder` nonEmpty.
### Why are the changes needed?
Code simplification.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Pass GitHub Actions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #38413: [SPARK-40936][SQL][TESTS] Refactor `AnalysisTest#assertAnalysisErrorClass` by reusing the `SparkFunSuite#checkError`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #38413:
URL: https://github.com/apache/spark/pull/38413#issuecomment-1294484128
Waiting for Ci.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #38413: [SPARK-40936][SQL][TESTS] Refactor `AnalysisTest#assertAnalysisErrorClass` by reusing the `SparkFunSuite#checkError`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #38413:
URL: https://github.com/apache/spark/pull/38413#issuecomment-1294915767
+1, LGTM. Merging to master.
Thank you, @LuciferYang.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38413: [SPARK-40936][SQL][TESTS] Remove outer conditions to simplify `AnalysisTest#assertAnalysisErrorClass` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38413:
URL: https://github.com/apache/spark/pull/38413#discussion_r1006825185
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisTest.scala:
##########
@@ -183,30 +185,28 @@ trait AnalysisTest extends PlanTest {
analyzer.checkAnalysis(analyzer.execute(inputPlan))
}
- if (e.getErrorClass != expectedErrorClass ||
- e.messageParameters != expectedMessageParameters ||
- (line >= 0 && e.line.getOrElse(-1) != line) ||
- (pos >= 0) && e.startPosition.getOrElse(-1) != pos) {
Review Comment:
It seems that the last condition was wrong:
```
(pos >= 0) && e.startPosition.getOrElse(-1) != pos
```
Should be
```
(pos >= 0 && e.startPosition.getOrElse(-1) != pos)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #38413: [SPARK-40936][SQL][TESTS] Remove outer conditions to simplify `AnalysisTest#assertAnalysisErrorClass` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #38413:
URL: https://github.com/apache/spark/pull/38413#issuecomment-1294431154
@MaxGekk
Refactor `assertAnalysisErrorClass` method:
- Reuse `checkError` in `assertAnalysisErrorClass`
- Use `queryContext` instead of `line + pos` in `assertAnalysisErrorClass` method signature
- Fixed related tests
Please review this if you have time, thanks ~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #38413: [SPARK-40936][SQL][TESTS] Remove outer conditions to simplify `AnalysisTest#assertAnalysisErrorClass` method
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #38413:
URL: https://github.com/apache/spark/pull/38413#issuecomment-1293723333
I wonder why do we need `assertAnalysisErrorClass()` at all. `checkError` does the same job. Seems like `assertAnalysisErrorClass()` checks additionally case sensitivity (can be done in test explicitly if it is really needed) + checking line + pos (we should check query context instead of that, I guess).
Let's consider to invoke `checkError` in `assertAnalysisErrorClass()` or remove it completely (invoke `checkError()` directly in tests as we do in other places).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk closed pull request #38413: [SPARK-40936][SQL][TESTS] Refactor `AnalysisTest#assertAnalysisErrorClass` by reusing the `SparkFunSuite#checkError`
Posted by GitBox <gi...@apache.org>.
MaxGekk closed pull request #38413: [SPARK-40936][SQL][TESTS] Refactor `AnalysisTest#assertAnalysisErrorClass` by reusing the `SparkFunSuite#checkError`
URL: https://github.com/apache/spark/pull/38413
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #38413: [SPARK-40936][SQL][TESTS] Refactor `AnalysisTest#assertAnalysisErrorClass` by reusing the `SparkFunSuite#checkError`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #38413:
URL: https://github.com/apache/spark/pull/38413#issuecomment-1294657123
GA passed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #38413: [SPARK-40936][SQL][TESTS] Remove outer conditions to simplify `AnalysisTest#assertAnalysisErrorClass` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #38413:
URL: https://github.com/apache/spark/pull/38413#issuecomment-1293745767
> assertAnalysisErrorClass
Sounds good, let me try. Set this to draft first and will ping you when it can be reviewed @MaxGekk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38413: [WIP][SPARK-40936][SQL][TESTS] Remove outer conditions to simplify `AnalysisTest#assertAnalysisErrorClass` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38413:
URL: https://github.com/apache/spark/pull/38413#discussion_r1007582019
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala:
##########
@@ -716,9 +714,8 @@ class AnalysisSuite extends AnalysisTest with Matchers {
assertAnalysisErrorClass(parsePlan("WITH t(x) AS (SELECT 1) SELECT * FROM t WHERE y = 1"),
"UNRESOLVED_COLUMN.WITH_SUGGESTION",
Map("objectName" -> "`y`", "proposal" -> "`t`.`x`"),
- caseSensitive = true,
- line = -1,
- pos = -1)
+ Array(ExpectedContext("y", 46, 46))
Review Comment:
https://github.com/apache/spark/blob/cf086b10de784fc92ae8b4d16065823ace520a7a/core/src/test/scala/org/apache/spark/SparkFunSuite.scala#L319-L332
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #38413: [SPARK-40936][SQL][TESTS] Remove outer conditions to simplify `AnalysisTest#assertAnalysisErrorClass` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #38413:
URL: https://github.com/apache/spark/pull/38413#issuecomment-1293477815
cc @MaxGekk @HyukjinKwon @dongjoon-hyun Think again, does this refactor look more simple?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38413: [SPARK-40936][SQL][TESTS] Remove outer conditions to simplify `AnalysisTest#assertAnalysisErrorClass` method
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38413:
URL: https://github.com/apache/spark/pull/38413#discussion_r1006825185
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisTest.scala:
##########
@@ -183,30 +185,28 @@ trait AnalysisTest extends PlanTest {
analyzer.checkAnalysis(analyzer.execute(inputPlan))
}
- if (e.getErrorClass != expectedErrorClass ||
- e.messageParameters != expectedMessageParameters ||
- (line >= 0 && e.line.getOrElse(-1) != line) ||
- (pos >= 0) && e.startPosition.getOrElse(-1) != pos) {
Review Comment:
It seems that the last condition was wrong? I think
```
(pos >= 0) && e.startPosition.getOrElse(-1) != pos
```
Should be
```
(pos >= 0 && e.startPosition.getOrElse(-1) != pos)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org