[SPARK-50449][SQL] Fix SQL Scripting grammar allowing empty bodies for loops, IF and CASE #48989
Conversation
| condition = "PARSE_SYNTAX_ERROR", | ||
| parameters = Map("error" -> "'LOOP'", "hint" -> "")) | ||
| } | ||
|
|
||
| test("loop with if else block") { |
There was a problem hiding this 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?
There was a problem hiding this 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.
There was a problem hiding this comment.
Added tests for FOR
| @@ -623,6 +652,21 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper { | |||
| assert(whileStmt.label.contains("lbl")) | |||
| } | |||
|
|
|||
| test("while with empty body") { | |||
There was a problem hiding this comment.
Can we also add test like the one in PR description:
WHILE 1=1 DO
BEGIN
END;
END WHILE;
There was a problem hiding this comment.
I will add that one in the follow up PR, as that case is still not fixed.
There was a problem hiding this comment.
@dusantism-db Could you open an JIRA and prepend it to PR's title, please.
dusantism-db
changed the title
|
@MaxGekk done |
|
@srielau PTAL |
| visitCompoundBodyImpl(ctx.compoundBody(), None, allowVarDeclare = true, labelCtx) | ||
| Option(ctx.compoundBody()) | ||
| .map(visitCompoundBodyImpl(_, None, allowVarDeclare = true, labelCtx)) | ||
| .getOrElse(CompoundBody(Seq.empty, None)) |
There was a problem hiding this comment.
I thought we should throw exception in the getOrElse branch, how does this fix work?
There was a problem hiding this 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.
dusantism-db
force-pushed
the
scripting-empty-bodies-fix
branch
from
98d40a7 to
1bb04a3
Compare
dusantism-db
force-pushed
the
scripting-empty-bodies-fix
branch
from
7495145 to
1cafcb5
Compare
|
thanks, merging to master! |
This PR depends on #48989 ### What changes were proposed in this pull request? There is a bug in SQL scripting which causes empty compound statements to throw an error if their body consists solely of empty BEGIN END blocks. Examples: ``` WHILE 1 = 1 DO BEGIN END; END WHILE; ``` ``` BEGIN BEGIN BEGIN END; END; END; ``` This PR fixes this by introducing a NO-OP statement for SQL scripting, which empty BEGIN END blocks will return. ### Why are the changes needed? Currenty, compound bodies declare they have the next element even if their body is consisted only of empty blocks. This is because it only checks for existence of statements in the body, not whether there is at least one statement which is not an empty block. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Unit tests were added to existing suites. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #49064 from dusantism-db/scripting-noop-statement. Authored-by: Dušan Tišma <dusan.tisma@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
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
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.