Skip to content

Commit

Permalink
[SPARK-50449][SQL] Fix SQL Scripting grammar allowing empty bodies fo…
Browse files Browse the repository at this point in the history
…r loops, IF and CASE

### 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, except this construct is correct grammar wise.
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.

Closes #48989 from dusantism-db/scripting-empty-bodies-fix.

Authored-by: Dušan Tišma <dusan.tisma@databricks.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
  • Loading branch information
dusantism-db authored and cloud-fan committed Dec 6, 2024
1 parent 851f5f2 commit c149942
Show file tree
Hide file tree
Showing 3 changed files with 145 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,15 @@ compoundOrSingleStatement
;

singleCompoundStatement
: BEGIN compoundBody END SEMICOLON? EOF
: BEGIN compoundBody? END SEMICOLON? EOF
;

beginEndCompoundBlock
: beginLabel? BEGIN compoundBody END endLabel?
: beginLabel? BEGIN compoundBody? END endLabel?
;

compoundBody
: (compoundStatements+=compoundStatement SEMICOLON)*
: (compoundStatements+=compoundStatement SEMICOLON)+
;

compoundStatement
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}

private def visitCompoundBodyImpl(
Expand Down Expand Up @@ -191,12 +193,9 @@ class AstBuilder extends DataTypeAstBuilder
labelCtx: SqlScriptingLabelContext): CompoundBody = {
val labelText =
labelCtx.enterLabeledScope(Option(ctx.beginLabel()), Option(ctx.endLabel()))
val body = visitCompoundBodyImpl(
ctx.compoundBody(),
Some(labelText),
allowVarDeclare = true,
labelCtx
)
val body = Option(ctx.compoundBody())
.map(visitCompoundBodyImpl(_, Some(labelText), allowVarDeclare = true, labelCtx))
.getOrElse(CompoundBody(Seq.empty, Some(labelText)))
labelCtx.exitLabeledScope(Option(ctx.beginLabel()))
body
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper {
}
}

test("empty BEGIN END block") {
test("empty singleCompoundStatement") {
val sqlScriptText =
"""
|BEGIN
Expand All @@ -91,6 +91,20 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper {
assert(tree.collection.isEmpty)
}

test("empty beginEndCompoundBlock") {
val sqlScriptText =
"""
|BEGIN
| BEGIN
| END;
|END""".stripMargin
val tree = parsePlan(sqlScriptText).asInstanceOf[CompoundBody]
assert(tree.collection.length == 1)
assert(tree.collection.head.isInstanceOf[CompoundBody])
val innerBody = tree.collection.head.asInstanceOf[CompoundBody]
assert(innerBody.collection.isEmpty)
}

test("multiple ; in row - should fail") {
val sqlScriptText =
"""
Expand Down Expand Up @@ -439,6 +453,21 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper {
assert(ifStmt.conditions.head.getText == "1=1")
}

test("if with empty body") {
val sqlScriptText =
"""BEGIN
| IF 1 = 1 THEN
| END IF;
|END
""".stripMargin
checkError(
exception = intercept[ParseException] {
parsePlan(sqlScriptText)
},
condition = "PARSE_SYNTAX_ERROR",
parameters = Map("error" -> "'IF'", "hint" -> ""))
}

test("if else") {
val sqlScriptText =
"""BEGIN
Expand Down Expand Up @@ -623,6 +652,21 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper {
assert(whileStmt.label.contains("lbl"))
}

test("while with empty body") {
val sqlScriptText =
"""BEGIN
| WHILE 1 = 1 DO
| END WHILE;
|END
""".stripMargin
checkError(
exception = intercept[ParseException] {
parsePlan(sqlScriptText)
},
condition = "PARSE_SYNTAX_ERROR",
parameters = Map("error" -> "'WHILE'", "hint" -> ""))
}

test("while with complex condition") {
val sqlScriptText =
"""
Expand Down Expand Up @@ -1067,6 +1111,21 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper {
assert(repeatStmt.label.contains("lbl"))
}

test("repeat with empty body") {
val sqlScriptText =
"""BEGIN
| REPEAT UNTIL 1 = 1
| END REPEAT;
|END
""".stripMargin
checkError(
exception = intercept[ParseException] {
parsePlan(sqlScriptText)
},
condition = "PARSE_SYNTAX_ERROR",
parameters = Map("error" -> "'1'", "hint" -> ""))
}

test("repeat with complex condition") {
val sqlScriptText =
"""
Expand Down Expand Up @@ -1197,6 +1256,22 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper {
assert(caseStmt.conditions.head.getText == "1 = 1")
}

test("searched case statement with empty body") {
val sqlScriptText =
"""BEGIN
| CASE
| WHEN 1 = 1 THEN
| END CASE;
|END
""".stripMargin
checkError(
exception = intercept[ParseException] {
parsePlan(sqlScriptText)
},
condition = "PARSE_SYNTAX_ERROR",
parameters = Map("error" -> "'CASE'", "hint" -> ""))
}

test("searched case statement - multi when") {
val sqlScriptText =
"""
Expand Down Expand Up @@ -1335,6 +1410,21 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper {
checkSimpleCaseStatementCondition(caseStmt.conditions.head, _ == Literal(1), _ == Literal(1))
}

test("simple case statement with empty body") {
val sqlScriptText =
"""BEGIN
| CASE 1
| WHEN 1 THEN
| END CASE;
|END
""".stripMargin
checkError(
exception = intercept[ParseException] {
parsePlan(sqlScriptText)
},
condition = "PARSE_SYNTAX_ERROR",
parameters = Map("error" -> "'CASE'", "hint" -> ""))
}

test("simple case statement - multi when") {
val sqlScriptText =
Expand Down Expand Up @@ -1482,6 +1572,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") {
val sqlScriptText =
"""BEGIN
Expand Down Expand Up @@ -1960,6 +2065,21 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper {
assert(forStmt.label.contains("lbl"))
}

test("for statement - empty body") {
val sqlScriptText =
"""
|BEGIN
| lbl: FOR x AS SELECT 5 DO
| END FOR;
|END""".stripMargin
checkError(
exception = intercept[ParseException] {
parsePlan(sqlScriptText)
},
condition = "PARSE_SYNTAX_ERROR",
parameters = Map("error" -> "'FOR'", "hint" -> ""))
}

test("for statement - no label") {
val sqlScriptText =
"""
Expand Down Expand Up @@ -2076,6 +2196,21 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper {
assert(forStmt.label.contains("lbl"))
}

test("for statement - no variable - empty body") {
val sqlScriptText =
"""
|BEGIN
| lbl: FOR SELECT 5 DO
| END FOR;
|END""".stripMargin
checkError(
exception = intercept[ParseException] {
parsePlan(sqlScriptText)
},
condition = "PARSE_SYNTAX_ERROR",
parameters = Map("error" -> "'FOR'", "hint" -> ""))
}

test("for statement - no variable - no label") {
val sqlScriptText =
"""
Expand Down

0 comments on commit c149942

Please sign in to comment.