You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "dusantism-db (via GitHub)" <gi...@apache.org> on 2024/11/27 15:28:05 UTC
[PR] [SQL] Fix SQL Scripting grammar allowing empty bodies for loops, IF and CASE [spark]
dusantism-db opened a new pull request, #48989:
URL: https://github.com/apache/spark/pull/48989
### What changes were proposed in this pull request?
Before this PR, SQL Scripting grammar allowed for loops, IF and CASE to have empty bodies. Example:
`WHILE 1 = 1 DO END WHILE;`
If they have an empty body, an internal error is thrown during execution. This PR changes the grammar so that loops, IF and CASE must have at least one statement in their bodies.
Note that this does not completely fix the internal error issue. It is still possible to have something like
```
WHILE 1 = 1 DO
BEGIN
END;
END WHILE;
```
where the same error is still thrown, and this is allowed according to the correct grammar.
This issue will be fixed by a separate PR, as non-trivial interpreter logic changes are required.
### Why are the changes needed?
The existing grammar was wrong.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Unit tests that make sure parsing loops, IF and CASE with empty bodies throws an error.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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
Re: [PR] [SPARK-50449][SQL] Fix SQL Scripting grammar allowing empty bodies for loops, IF and CASE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #48989:
URL: https://github.com/apache/spark/pull/48989#issuecomment-2522099128
thanks, merging to master!
--
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
Re: [PR] [SPARK-50449][SQL] Fix SQL Scripting grammar allowing empty bodies for loops, IF and CASE [spark]
Posted by "dusantism-db (via GitHub)" <gi...@apache.org>.
dusantism-db commented on code in PR #48989:
URL: https://github.com/apache/spark/pull/48989#discussion_r1870325852
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/SqlScriptingParserSuite.scala:
##########
@@ -623,6 +652,21 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper {
assert(whileStmt.label.contains("lbl"))
}
+ test("while with empty body") {
Review Comment:
Added tests for FOR
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/SqlScriptingParserSuite.scala:
##########
@@ -1483,6 +1573,21 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper {
assert(whileStmt.label.contains("lbl"))
}
+ test("loop with empty body") {
+ val sqlScriptText =
+ """BEGIN
+ | LOOP
+ | END LOOP;
+ |END
+ """.stripMargin
+ checkError(
+ exception = intercept[ParseException] {
+ parsePlan(sqlScriptText)
+ },
+ condition = "PARSE_SYNTAX_ERROR",
+ parameters = Map("error" -> "'LOOP'", "hint" -> ""))
+ }
+
test("loop with if else block") {
Review Comment:
Added tests for FOR
--
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
Re: [PR] [SPARK-50449][SQL] Fix SQL Scripting grammar allowing empty bodies for loops, IF and CASE [spark]
Posted by "dusantism-db (via GitHub)" <gi...@apache.org>.
dusantism-db commented on PR #48989:
URL: https://github.com/apache/spark/pull/48989#issuecomment-2506013765
@MaxGekk done
--
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
Re: [PR] [SQL] Fix SQL Scripting grammar allowing empty bodies for loops, IF and CASE [spark]
Posted by "dusantism-db (via GitHub)" <gi...@apache.org>.
dusantism-db commented on code in PR #48989:
URL: https://github.com/apache/spark/pull/48989#discussion_r1860943556
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/SqlScriptingParserSuite.scala:
##########
@@ -623,6 +652,21 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper {
assert(whileStmt.label.contains("lbl"))
}
+ test("while with empty body") {
Review Comment:
I will add that one in the follow up PR, as that case is still not fixed.
--
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
Re: [PR] [SPARK-50449][SQL] Fix SQL Scripting grammar allowing empty bodies for loops, IF and CASE [spark]
Posted by "dusantism-db (via GitHub)" <gi...@apache.org>.
dusantism-db commented on code in PR #48989:
URL: https://github.com/apache/spark/pull/48989#discussion_r1870034178
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -144,7 +144,9 @@ class AstBuilder extends DataTypeAstBuilder
override def visitSingleCompoundStatement(ctx: SingleCompoundStatementContext): CompoundBody = {
val labelCtx = new SqlScriptingLabelContext()
- visitCompoundBodyImpl(ctx.compoundBody(), None, allowVarDeclare = true, labelCtx)
+ Option(ctx.compoundBody())
+ .map(visitCompoundBodyImpl(_, None, allowVarDeclare = true, labelCtx))
+ .getOrElse(CompoundBody(Seq.empty, None))
Review Comment:
If `ctx.compoundBody()` is null, that means we have
```
BEGIN
END;
```
This is allowed, so in this case we simply return a `CompoundBody` with empty statements list.
--
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
Re: [PR] [SQL] Fix SQL Scripting grammar allowing empty bodies for loops, IF and CASE [spark]
Posted by "dejankrak-db (via GitHub)" <gi...@apache.org>.
dejankrak-db commented on code in PR #48989:
URL: https://github.com/apache/spark/pull/48989#discussion_r1860873704
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/SqlScriptingParserSuite.scala:
##########
@@ -1483,6 +1573,21 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper {
assert(whileStmt.label.contains("lbl"))
}
+ test("loop with empty body") {
+ val sqlScriptText =
+ """BEGIN
+ | LOOP
+ | END LOOP;
+ |END
+ """.stripMargin
+ checkError(
+ exception = intercept[ParseException] {
+ parsePlan(sqlScriptText)
+ },
+ condition = "PARSE_SYNTAX_ERROR",
+ parameters = Map("error" -> "'LOOP'", "hint" -> ""))
+ }
+
test("loop with if else block") {
Review Comment:
Does it also make sense to add test with for with empty body, in this PR or your other one, depending on which you merge first?
--
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
Re: [PR] [SPARK-50449][SQL] Fix SQL Scripting grammar allowing empty bodies for loops, IF and CASE [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #48989:
URL: https://github.com/apache/spark/pull/48989#issuecomment-2517621048
@srielau PTAL
--
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
Re: [PR] [SQL] Fix SQL Scripting grammar allowing empty bodies for loops, IF and CASE [spark]
Posted by "miland-db (via GitHub)" <gi...@apache.org>.
miland-db commented on code in PR #48989:
URL: https://github.com/apache/spark/pull/48989#discussion_r1860885518
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/SqlScriptingParserSuite.scala:
##########
@@ -623,6 +652,21 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper {
assert(whileStmt.label.contains("lbl"))
}
+ test("while with empty body") {
Review Comment:
Can we also add test like the one in PR description:
```
WHILE
BEGIN
END;
END WHILE;
```
--
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
Re: [PR] [SPARK-50449][SQL] Fix SQL Scripting grammar allowing empty bodies for loops, IF and CASE [spark]
Posted by "dusantism-db (via GitHub)" <gi...@apache.org>.
dusantism-db commented on code in PR #48989:
URL: https://github.com/apache/spark/pull/48989#discussion_r1870325852
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/SqlScriptingParserSuite.scala:
##########
@@ -623,6 +652,21 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper {
assert(whileStmt.label.contains("lbl"))
}
+ test("while with empty body") {
Review Comment:
Added tests for FOR
--
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
Re: [PR] [SPARK-50449][SQL] Fix SQL Scripting grammar allowing empty bodies for loops, IF and CASE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #48989:
URL: https://github.com/apache/spark/pull/48989#discussion_r1869823693
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -144,7 +144,9 @@ class AstBuilder extends DataTypeAstBuilder
override def visitSingleCompoundStatement(ctx: SingleCompoundStatementContext): CompoundBody = {
val labelCtx = new SqlScriptingLabelContext()
- visitCompoundBodyImpl(ctx.compoundBody(), None, allowVarDeclare = true, labelCtx)
+ Option(ctx.compoundBody())
+ .map(visitCompoundBodyImpl(_, None, allowVarDeclare = true, labelCtx))
+ .getOrElse(CompoundBody(Seq.empty, None))
Review Comment:
I thought we should throw exception in the `getOrElse` branch, how does this fix work?
--
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
Re: [PR] [SPARK-50449][SQL] Fix SQL Scripting grammar allowing empty bodies for loops, IF and CASE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #48989: [SPARK-50449][SQL] Fix SQL Scripting grammar allowing empty bodies for loops, IF and CASE
URL: https://github.com/apache/spark/pull/48989
--
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
Re: [PR] [SQL] Fix SQL Scripting grammar allowing empty bodies for loops, IF and CASE [spark]
Posted by "dusantism-db (via GitHub)" <gi...@apache.org>.
dusantism-db commented on code in PR #48989:
URL: https://github.com/apache/spark/pull/48989#discussion_r1860884219
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/SqlScriptingParserSuite.scala:
##########
@@ -1483,6 +1573,21 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper {
assert(whileStmt.label.contains("lbl"))
}
+ test("loop with empty body") {
+ val sqlScriptText =
+ """BEGIN
+ | LOOP
+ | END LOOP;
+ |END
+ """.stripMargin
+ checkError(
+ exception = intercept[ParseException] {
+ parsePlan(sqlScriptText)
+ },
+ condition = "PARSE_SYNTAX_ERROR",
+ parameters = Map("error" -> "'LOOP'", "hint" -> ""))
+ }
+
test("loop with if else block") {
Review Comment:
Yes, I will add it to the one which gets merged last, or if they both get merged then I will create a separate PR.
--
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
Re: [PR] [SQL] Fix SQL Scripting grammar allowing empty bodies for loops, IF and CASE [spark]
Posted by "miland-db (via GitHub)" <gi...@apache.org>.
miland-db commented on code in PR #48989:
URL: https://github.com/apache/spark/pull/48989#discussion_r1860885518
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/SqlScriptingParserSuite.scala:
##########
@@ -623,6 +652,21 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper {
assert(whileStmt.label.contains("lbl"))
}
+ test("while with empty body") {
Review Comment:
Can we also add test like the one in PR description:
```
WHILE 1=1 DO
BEGIN
END;
END WHILE;
```
--
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