Skip to content

[SPARK-50449][SQL] Fix SQL Scripting grammar allowing empty bodies for loops, IF and CASE #48989

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.

Sign up for GitHub

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

Closed
dusantism-db wants to merge 5 commits into from
Closed

[SPARK-50449][SQL] Fix SQL Scripting grammar allowing empty bodies for loops, IF and CASE #48989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
50a8061
fix grammar to not support empty bodies in loops/if/case
dusantism-db Nov 26, 2024
7a41e6b
add tests for empty loops
dusantism-db Nov 27, 2024
729e735
format
dusantism-db Nov 27, 2024
2260faa
fix parser test
dusantism-db Nov 27, 2024
1cafcb5
add tests for for statement
dusantism-db Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4
View file
Open in desktop
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
13 changes: 6 additions & 7 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
View file
Open in desktop
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))
Copy link
Copy Markdown
Contributor

@cloud-fan cloud-fan Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we should throw exception in the getOrElse branch, how does this fix work?

Copy link
Copy Markdown
Contributor Author

@dusantism-db dusantism-db Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

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
137 changes: 136 additions & 1 deletion ...atalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/SqlScriptingParserSuite.scala
View file
Open in desktop
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") {
Copy link
Copy Markdown
Contributor

@miland-db miland-db Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add test like the one in PR description:

WHILE 1=1 DO
  BEGIN
  END;
END WHILE;

Copy link
Copy Markdown
Contributor Author

@dusantism-db dusantism-db Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add that one in the follow up PR, as that case is still not fixed.

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") {
Copy link
Copy Markdown
Contributor

@dejankrak-db dejankrak-db Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@dusantism-db dusantism-db Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@dusantism-db dusantism-db Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests for FOR

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