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/11/09 05:48:25 UTC
[GitHub] [spark] itholic opened a new pull request, #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
itholic opened a new pull request, #38576:
URL: https://github.com/apache/spark/pull/38576
### What changes were proposed in this pull request?
This PR proposes to rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`.
### Why are the changes needed?
The sub-error class name is duplicated with its main class, `UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY`.
We should make the all error class name clear and briefly.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
```
./build/sbt “sql/testOnly org.apache.spark.sql.SQLQueryTestSuite*”
```
--
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] itholic commented on a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1023536354
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1277,6 +1277,11 @@
"A correlated outer name reference within a subquery expression body was not found in the enclosing query: <value>"
]
},
+ "CORRELATED_REFERENCE" : {
+ "message" : [
+ "Expressions referencing the outer query are not supported outside of WHERE/HAVING clauses<treeNode>"
Review Comment:
Yes, just fixed it to show SQL expression!
--
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] srielau commented on a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1044041728
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1443,6 +1443,11 @@
"A correlated outer name reference within a subquery expression body was not found in the enclosing query: <value>"
]
},
+ "CORRELATED_REFERENCE" : {
+ "message" : [
+ "Expressions <sqlExprs> referencing the outer query are not supported outside of WHERE/HAVING clauses."
Review Comment:
Are we really returning a set of expressions? Or is the plural accidental?
--
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] itholic commented on a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1036705541
##########
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala:
##########
@@ -964,14 +964,16 @@ class SubquerySuite extends QueryTest
| WHERE t1.c1 = t2.c1)
""".stripMargin)
}
- checkError(
+ checkErrorMatchPVals(
exception1,
errorClass = "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.CORRELATED_REFERENCE",
- parameters = Map("sqlExprs" -> "outer(arr_c2#427264)"),
+ sqlState = None,
+ parameters = Map("sqlExprs" -> "explode\\(outer\\(arr_c2#.*"),
Review Comment:
I squeeze out the outer expression here, but seems like this shows `explode(arr_c2)` with current behavior of `stripOuterReference`.
@MaxGekk WDYT? Is there any way, or should we fix the `stripOuterReference` to display the `t2.arr_c2` here?
--
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 a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1033288326
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -1059,11 +1059,12 @@ trait CheckAnalysis extends PredicateHelper with LookupCatalog with QueryErrorsB
// 2. Expressions containing outer references on plan nodes other than allowed operators.
def failOnInvalidOuterReference(p: LogicalPlan): Unit = {
p.expressions.foreach(checkMixedReferencesInsideAggregateExpr)
- if (!canHostOuter(p) && p.expressions.exists(containsOuter)) {
+ val exprs = p.expressions.filter(expr => containsOuter(expr))
+ if (!canHostOuter(p) && !exprs.isEmpty) {
p.failAnalysis(
errorClass =
- "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.UNSUPPORTED_CORRELATED_REFERENCE",
- messageParameters = Map("treeNode" -> planToString(p)))
+ "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.CORRELATED_REFERENCE",
+ messageParameters = Map("sqlExprs" -> exprs.mkString(",")))
Review Comment:
Please, use `toSQLExpr` to quote expressions.
--
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] itholic commented on a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1046656616
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1443,6 +1443,11 @@
"A correlated outer name reference within a subquery expression body was not found in the enclosing query: <value>"
]
},
+ "CORRELATED_REFERENCE" : {
+ "message" : [
+ "Expressions <sqlExprs> referencing the outer query are not supported outside of WHERE/HAVING clauses."
Review Comment:
> Can we add the sqlExprs at the end of the error message instead of inserting them in the middle?
Sounds good. Let me address the comment!
--
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 #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #38576:
URL: https://github.com/apache/spark/pull/38576#issuecomment-1349511043
+1, LGTM. Merging to master.
Thank you, @itholic.
--
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 a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1035007704
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1388,6 +1388,11 @@
"A correlated outer name reference within a subquery expression body was not found in the enclosing query: <value>"
]
},
+ "CORRELATED_REFERENCE" : {
+ "message" : [
+ "Expressions '<sqlExprs>' referencing the outer query are not supported outside of WHERE/HAVING clauses."
Review Comment:
`'<sqlExprs>'` -> `<sqlExprs>`
--
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 a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1018717817
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1277,6 +1277,11 @@
"A correlated outer name reference within a subquery expression body was not found in the enclosing query: <value>"
]
},
+ "CORRELATED_REFERENCE" : {
+ "message" : [
+ "Expressions referencing the outer query are not supported outside of WHERE/HAVING clauses<treeNode>"
Review Comment:
Let's don't confuse users by `treeNode`, and show something more useful like problematic SQL expr.
--
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] itholic commented on a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1036693778
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveSubquerySuite.scala:
##########
@@ -56,7 +56,8 @@ class ResolveSubquerySuite extends AnalysisTest {
SimpleAnalyzer.checkAnalysis(SimpleAnalyzer.ResolveSubquery(expr))
},
errorClass = "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.CORRELATED_REFERENCE",
- parameters = Map("sqlExprs" -> "outer(a#4151672)")
+ parameters = Map("sqlExprs" -> "outer\\(a#.*"),
Review Comment:
Yes, we can. Let me address it.
Thanks for the comment!
--
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 a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1047520624
##########
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala:
##########
@@ -964,14 +964,16 @@ class SubquerySuite extends QueryTest
| WHERE t1.c1 = t2.c1)
""".stripMargin)
}
- checkError(
+ checkErrorMatchPVals(
exception1,
errorClass = "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.CORRELATED_REFERENCE",
- parameters = Map("sqlExprs" -> "outer(arr_c2#427264)"),
+ sqlState = None,
+ parameters = Map("sqlExprs" -> "explode\\(outer\\(arr_c2#.*"),
Review Comment:
> should we fix the stripOuterReference to display the t2.arr_c2 here?
No, let's fix it separately.
--
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] itholic commented on a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1025069390
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -1059,8 +1059,8 @@ trait CheckAnalysis extends PredicateHelper with LookupCatalog with QueryErrorsB
if (!canHostOuter(p) && p.expressions.exists(containsOuter)) {
p.failAnalysis(
errorClass =
- "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.UNSUPPORTED_CORRELATED_REFERENCE",
- messageParameters = Map("treeNode" -> planToString(p)))
+ "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.CORRELATED_REFERENCE",
+ messageParameters = Map("sqlExprs" -> p.expressions.map(_.sql).mkString(",")))
Review Comment:
Oh, I got it. Let me address it.
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] itholic commented on a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1028810170
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -1059,10 +1060,16 @@ trait CheckAnalysis extends PredicateHelper with LookupCatalog with QueryErrorsB
def failOnInvalidOuterReference(p: LogicalPlan): Unit = {
p.expressions.foreach(checkMixedReferencesInsideAggregateExpr)
if (!canHostOuter(p) && p.expressions.exists(containsOuter)) {
+ val exprs = new ListBuffer[String]()
+ for (expr <- p.expressions) {
Review Comment:
Thanks! Just addressed the comments
--
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 a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1024229060
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -1059,8 +1059,8 @@ trait CheckAnalysis extends PredicateHelper with LookupCatalog with QueryErrorsB
if (!canHostOuter(p) && p.expressions.exists(containsOuter)) {
p.failAnalysis(
errorClass =
- "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.UNSUPPORTED_CORRELATED_REFERENCE",
- messageParameters = Map("treeNode" -> planToString(p)))
+ "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.CORRELATED_REFERENCE",
+ messageParameters = Map("sqlExprs" -> p.expressions.map(_.sql).mkString(",")))
Review Comment:
Could you show only expressions for which `containsOuter()` returns `true`.
--
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] itholic commented on a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1030417017
##########
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala:
##########
@@ -964,17 +964,14 @@ class SubquerySuite extends QueryTest
| WHERE t1.c1 = t2.c1)
""".stripMargin)
}
- checkErrorMatchPVals(
+ checkError(
exception1,
- errorClass = "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.UNSUPPORTED_CORRELATED_REFERENCE",
- parameters = Map("treeNode" -> "(?s).*"),
- sqlState = None,
+ errorClass = "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.CORRELATED_REFERENCE",
+ parameters = Map("sqlExprs" -> "outer(arr_c2#427264)"),
Review Comment:
Thanks for catching out, seems like we should use `matchPVals` here. Let me address
--
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] srielau commented on a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1030591230
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveSubquerySuite.scala:
##########
@@ -56,7 +56,8 @@ class ResolveSubquerySuite extends AnalysisTest {
SimpleAnalyzer.checkAnalysis(SimpleAnalyzer.ResolveSubquery(expr))
},
errorClass = "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.CORRELATED_REFERENCE",
- parameters = Map("sqlExprs" -> "outer(a#4151672)")
+ parameters = Map("sqlExprs" -> "outer\\(a#.*"),
Review Comment:
outer() is an internal node, can we squeeze it out?
Don't we have access to context here to get the "real" expression?
We should only show `a` here.
--
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 #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
MaxGekk closed pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
URL: https://github.com/apache/spark/pull/38576
--
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 a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1026031578
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -1059,10 +1060,16 @@ trait CheckAnalysis extends PredicateHelper with LookupCatalog with QueryErrorsB
def failOnInvalidOuterReference(p: LogicalPlan): Unit = {
p.expressions.foreach(checkMixedReferencesInsideAggregateExpr)
if (!canHostOuter(p) && p.expressions.exists(containsOuter)) {
+ val exprs = new ListBuffer[String]()
+ for (expr <- p.expressions) {
Review Comment:
This can be simpler using `filter` instead of `exists` and `for`
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -1059,10 +1060,16 @@ trait CheckAnalysis extends PredicateHelper with LookupCatalog with QueryErrorsB
def failOnInvalidOuterReference(p: LogicalPlan): Unit = {
p.expressions.foreach(checkMixedReferencesInsideAggregateExpr)
if (!canHostOuter(p) && p.expressions.exists(containsOuter)) {
+ val exprs = new ListBuffer[String]()
+ for (expr <- p.expressions) {
+ if (containsOuter(expr)) {
+ exprs += toPrettySQL(expr)
Review Comment:
Use `toSQLExpr`
--
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] itholic commented on a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1021045305
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1277,6 +1277,11 @@
"A correlated outer name reference within a subquery expression body was not found in the enclosing query: <value>"
]
},
+ "CORRELATED_REFERENCE" : {
+ "message" : [
+ "Expressions referencing the outer query are not supported outside of WHERE/HAVING clauses<treeNode>"
Review Comment:
@MaxGekk May I ask if there is any way to extract the SQL expr from LogicalPlan ??
I'm trying to switch `planToString(p)` to SQL expression but still not sure how.
--
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 a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1021741191
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1277,6 +1277,11 @@
"A correlated outer name reference within a subquery expression body was not found in the enclosing query: <value>"
]
},
+ "CORRELATED_REFERENCE" : {
+ "message" : [
+ "Expressions referencing the outer query are not supported outside of WHERE/HAVING clauses<treeNode>"
Review Comment:
> Expressions referencing the outer query ...
Can you show the expression?
--
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] itholic commented on a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1030417017
##########
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala:
##########
@@ -964,17 +964,14 @@ class SubquerySuite extends QueryTest
| WHERE t1.c1 = t2.c1)
""".stripMargin)
}
- checkErrorMatchPVals(
+ checkError(
exception1,
- errorClass = "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.UNSUPPORTED_CORRELATED_REFERENCE",
- parameters = Map("treeNode" -> "(?s).*"),
- sqlState = None,
+ errorClass = "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.CORRELATED_REFERENCE",
+ parameters = Map("sqlExprs" -> "outer(arr_c2#427264)"),
Review Comment:
Thanks for catching out, seems like we should use regex here. Let me address
--
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] srielau commented on a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1019261584
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1277,6 +1277,11 @@
"A correlated outer name reference within a subquery expression body was not found in the enclosing query: <value>"
]
},
+ "CORRELATED_REFERENCE" : {
+ "message" : [
+ "Expressions referencing the outer query are not supported outside of WHERE/HAVING clauses<treeNode>"
Review Comment:
Agreed, let's show the expression. (having the expression in the context would also be acceptable). No treenode
--
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 a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1030039032
##########
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala:
##########
@@ -964,17 +964,14 @@ class SubquerySuite extends QueryTest
| WHERE t1.c1 = t2.c1)
""".stripMargin)
}
- checkErrorMatchPVals(
+ checkError(
exception1,
- errorClass = "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.UNSUPPORTED_CORRELATED_REFERENCE",
- parameters = Map("treeNode" -> "(?s).*"),
- sqlState = None,
+ errorClass = "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.CORRELATED_REFERENCE",
+ parameters = Map("sqlExprs" -> "outer(arr_c2#427264)"),
Review Comment:
the same question here. Can we guarantee the same number across multiple runs.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveSubquerySuite.scala:
##########
@@ -51,11 +51,13 @@ class ResolveSubquerySuite extends AnalysisTest {
test("SPARK-17251 Improve `OuterReference` to be `NamedExpression`") {
val expr = Filter(
InSubquery(Seq(a), ListQuery(Project(Seq(UnresolvedAttribute("a")), t2))), t1)
- val m = intercept[AnalysisException] {
- SimpleAnalyzer.checkAnalysis(SimpleAnalyzer.ResolveSubquery(expr))
- }.getMessage
- assert(m.contains(
- "Expressions referencing the outer query are not supported outside of WHERE/HAVING clauses"))
+ checkError(
+ exception = intercept[AnalysisException] {
+ SimpleAnalyzer.checkAnalysis(SimpleAnalyzer.ResolveSubquery(expr))
+ },
+ errorClass = "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.CORRELATED_REFERENCE",
+ parameters = Map("sqlExprs" -> "outer(a#4151672)")
Review Comment:
Is the number `4151672` stable?
--
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] srielau commented on a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1030592473
##########
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala:
##########
@@ -964,14 +964,16 @@ class SubquerySuite extends QueryTest
| WHERE t1.c1 = t2.c1)
""".stripMargin)
}
- checkError(
+ checkErrorMatchPVals(
exception1,
errorClass = "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.CORRELATED_REFERENCE",
- parameters = Map("sqlExprs" -> "outer(arr_c2#427264)"),
+ sqlState = None,
+ parameters = Map("sqlExprs" -> "explode\\(outer\\(arr_c2#.*"),
Review Comment:
This should be: `t2`.`arr_c2`
--
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] allisonwang-db commented on a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
allisonwang-db commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1043949188
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1443,6 +1443,11 @@
"A correlated outer name reference within a subquery expression body was not found in the enclosing query: <value>"
]
},
+ "CORRELATED_REFERENCE" : {
+ "message" : [
+ "Expressions <sqlExprs> referencing the outer query are not supported outside of WHERE/HAVING clauses."
Review Comment:
Can we add the sqlExprs at the end of the error message instead of inserting them in the middle?
```suggestion
"Expressions referencing the outer query are not supported outside of WHERE/HAVING clauses: <sqlExprs>"
```
--
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] itholic commented on pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
itholic commented on PR #38576:
URL: https://github.com/apache/spark/pull/38576#issuecomment-1309637126
cc @MaxGekk @srielau
--
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 a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1024817626
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -1059,8 +1059,8 @@ trait CheckAnalysis extends PredicateHelper with LookupCatalog with QueryErrorsB
if (!canHostOuter(p) && p.expressions.exists(containsOuter)) {
p.failAnalysis(
errorClass =
- "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.UNSUPPORTED_CORRELATED_REFERENCE",
- messageParameters = Map("treeNode" -> planToString(p)))
+ "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.CORRELATED_REFERENCE",
+ messageParameters = Map("sqlExprs" -> p.expressions.map(_.sql).mkString(",")))
Review Comment:
`exists()` returns `true` if at least ONE element satisfies the requirement but you show ALL.
--
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] itholic commented on a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1024711225
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -1059,8 +1059,8 @@ trait CheckAnalysis extends PredicateHelper with LookupCatalog with QueryErrorsB
if (!canHostOuter(p) && p.expressions.exists(containsOuter)) {
p.failAnalysis(
errorClass =
- "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.UNSUPPORTED_CORRELATED_REFERENCE",
- messageParameters = Map("treeNode" -> planToString(p)))
+ "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.CORRELATED_REFERENCE",
+ messageParameters = Map("sqlExprs" -> p.expressions.map(_.sql).mkString(",")))
Review Comment:
Maybe it's a dumb question, but it's already checked in `if` clause above ??
```
if (!canHostOuter(p) && p.expressions.exists(containsOuter)) {
```
Is it different one?
--
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] itholic commented on a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1046656452
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1443,6 +1443,11 @@
"A correlated outer name reference within a subquery expression body was not found in the enclosing query: <value>"
]
},
+ "CORRELATED_REFERENCE" : {
+ "message" : [
+ "Expressions <sqlExprs> referencing the outer query are not supported outside of WHERE/HAVING clauses."
Review Comment:
Yes, it really returns a set of expressions separated by `,`
In `CheckAnalysis.scala`, we make the returning `sqlExprs`:
```scala
messageParameters = Map("sqlExprs" -> exprs.map(toSQLExpr).mkString(",")))
```
--
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] itholic commented on a diff in pull request #38576: [SPARK-41062][SQL] Rename `UNSUPPORTED_CORRELATED_REFERENCE` to `CORRELATED_REFERENCE`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38576:
URL: https://github.com/apache/spark/pull/38576#discussion_r1036705541
##########
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala:
##########
@@ -964,14 +964,16 @@ class SubquerySuite extends QueryTest
| WHERE t1.c1 = t2.c1)
""".stripMargin)
}
- checkError(
+ checkErrorMatchPVals(
exception1,
errorClass = "UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.CORRELATED_REFERENCE",
- parameters = Map("sqlExprs" -> "outer(arr_c2#427264)"),
+ sqlState = None,
+ parameters = Map("sqlExprs" -> "explode\\(outer\\(arr_c2#.*"),
Review Comment:
I squeeze out the outer expression here, but seems like this shows `explode(arr_c2)` with current behavior of `stripOuterReference`.
@MaxGekk Do you happen to know is there any way, or should we fix the `stripOuterReference` to display the `t2.arr_c2` here?
--
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