Skip to content

Commit

Permalink
[SPARK-50491][SQL] Fix bug where empty BEGIN END blocks throw an error
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@dusantism-db @cloud-fan
dusantism-db authored and cloud-fan committed Dec 10, 2024
1 parent 559fda7 commit e70f8ab
Show file tree
Hide file tree
Showing 3 changed files with 272 additions and 4 deletions.
8 changes: 8 additions & 0 deletions sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionNode.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,14 @@ class SingleStatementExec(
override def reset(): Unit = isExecuted = false
}

/**
* NO-OP leaf node, which does nothing when returned to the iterator.
* It is emitted by empty BEGIN END blocks.
*/
class NoOpStatementExec extends LeafStatementExec {
override def reset(): Unit = ()
}

/**
* Executable node for CompoundBody.
* @param statements
Expand Down
11 changes: 8 additions & 3 deletions sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingInterpreter.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,14 @@ case class SqlScriptingInterpreter(session: SparkSession) {
.map(varName => DropVariable(varName, ifExists = true))
.map(new SingleStatementExec(_, Origin(), args, isInternal = true))
.reverse
new CompoundBodyExec(
collection.map(st => transformTreeIntoExecutable(st, args)) ++ dropVariables,
label)

val statements =
collection.map(st => transformTreeIntoExecutable(st, args)) ++ dropVariables match {
case Nil => Seq(new NoOpStatementExec)
case s => s
}

new CompoundBodyExec(statements, label)

case IfElseStatement(conditions, conditionalBodies, elseBody) =>
val conditionsExec = conditions.map(condition =>
Expand Down
257 changes: 256 additions & 1 deletion sql/core/src/test/scala/org/apache/spark/sql/scripting/SqlScriptingInterpreterSuite.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,61 @@ class SqlScriptingInterpreterSuite extends QueryTest with SharedSparkSession {
}
}

test("empty begin end block") {
val sqlScript =
"""
|BEGIN
|END
|""".stripMargin
val expected = Seq.empty[Seq[Row]]
verifySqlScriptResult(sqlScript, expected)
}

test("empty begin end blocks") {
val sqlScript =
"""
|BEGIN
| BEGIN
| END;
| BEGIN
| END;
|END
|""".stripMargin
val expected = Seq.empty[Seq[Row]]
verifySqlScriptResult(sqlScript, expected)
}

test("empty begin end blocks with single statement") {
val sqlScript =
"""
|BEGIN
| BEGIN
| END;
| SELECT 1;
| BEGIN
| END;
|END
|""".stripMargin
val expected = Seq(Seq(Row(1)))
verifySqlScriptResult(sqlScript, expected)
}

test("empty begin end blocks - nested") {
val sqlScript =
"""
|BEGIN
| BEGIN
| BEGIN
| END;
| BEGIN
| END;
| END;
|END
|""".stripMargin
val expected = Seq.empty[Seq[Row]]
verifySqlScriptResult(sqlScript, expected)
}

test("session vars - set and read (SET VAR)") {
val sqlScript =
"""
Expand Down Expand Up @@ -240,6 +295,40 @@ class SqlScriptingInterpreterSuite extends QueryTest with SharedSparkSession {
verifySqlScriptResult(commands, expected)
}

test("if - empty body") {
val commands =
"""
|BEGIN
| IF 1=1 THEN
| BEGIN
| END;
| END IF;
|END
|""".stripMargin
val expected = Seq.empty[Seq[Row]]
verifySqlScriptResult(commands, expected)
}

test("if - nested empty body") {
val commands =
"""
|BEGIN
| IF 1=1 THEN
| BEGIN
| BEGIN
| END;
| END;
| BEGIN
| BEGIN
| END;
| END;
| END IF;
|END
|""".stripMargin
val expected = Seq.empty[Seq[Row]]
verifySqlScriptResult(commands, expected)
}

test("if nested") {
val commands =
"""
Expand Down Expand Up @@ -389,6 +478,42 @@ class SqlScriptingInterpreterSuite extends QueryTest with SharedSparkSession {
verifySqlScriptResult(commands, expected)
}

test("searched case - empty body") {
val commands =
"""
|BEGIN
| CASE
| WHEN 1 = 1 THEN
| BEGIN
| END;
| END CASE;
|END
|""".stripMargin
val expected = Seq.empty[Seq[Row]]
verifySqlScriptResult(commands, expected)
}

test("searched case - nested empty body") {
val commands =
"""
|BEGIN
| CASE
| WHEN 1 = 1 THEN
| BEGIN
| BEGIN
| END;
| END;
| BEGIN
| BEGIN
| END;
| END;
| END CASE;
|END
|""".stripMargin
val expected = Seq.empty[Seq[Row]]
verifySqlScriptResult(commands, expected)
}

test("searched case nested") {
val commands =
"""
Expand Down Expand Up @@ -589,6 +714,42 @@ class SqlScriptingInterpreterSuite extends QueryTest with SharedSparkSession {
verifySqlScriptResult(commands, expected)
}

test("simple case - empty body") {
val commands =
"""
|BEGIN
| CASE 1
| WHEN 1 THEN
| BEGIN
| END;
| END CASE;
|END
|""".stripMargin
val expected = Seq.empty[Seq[Row]]
verifySqlScriptResult(commands, expected)
}

test("simple case - nested empty body") {
val commands =
"""
|BEGIN
| CASE 1
| WHEN 1 THEN
| BEGIN
| BEGIN
| END;
| END;
| BEGIN
| BEGIN
| END;
| END;
| END CASE;
|END
|""".stripMargin
val expected = Seq.empty[Seq[Row]]
verifySqlScriptResult(commands, expected)
}

test("simple case nested") {
val commands =
"""
Expand Down Expand Up @@ -985,6 +1146,42 @@ class SqlScriptingInterpreterSuite extends QueryTest with SharedSparkSession {
verifySqlScriptResult(commands, expected)
}

test("repeat - empty body") {
val commands =
"""
|BEGIN
| REPEAT
| BEGIN
| END;
| UNTIL 1 = 1
| END REPEAT;
|END
|""".stripMargin

val expected = Seq.empty[Seq[Row]]
verifySqlScriptResult(commands, expected)
}

test("repeat - nested empty body") {
val commands =
"""
|BEGIN
| REPEAT
| BEGIN
| BEGIN
| END;
| END;
| BEGIN
| END;
| UNTIL 1 = 1
| END REPEAT;
|END
|""".stripMargin

val expected = Seq.empty[Seq[Row]]
verifySqlScriptResult(commands, expected)
}

test("nested repeat") {
val commands =
"""
Expand Down Expand Up @@ -1795,7 +1992,7 @@ class SqlScriptingInterpreterSuite extends QueryTest with SharedSparkSession {
}
}

test("for statement empty result") {
test("for statement - empty result") {
withTable("t") {
val sqlScript =
"""
Expand All @@ -1814,6 +2011,64 @@ class SqlScriptingInterpreterSuite extends QueryTest with SharedSparkSession {
}
}

test("for statement - empty body") {
withTable("t") {
val sqlScript =
"""
|BEGIN
| CREATE TABLE t (intCol INT, stringCol STRING, doubleCol DOUBLE) using parquet;
| INSERT INTO t VALUES (1, 'first', 1.0);
| FOR row AS SELECT * FROM t DO
| BEGIN
| END;
| END FOR;
|END
|""".stripMargin

val expected = Seq(
Seq.empty[Row], // create table
Seq.empty[Row], // insert
Seq.empty[Row], // drop local var
Seq.empty[Row], // drop local var
Seq.empty[Row], // drop local var
Seq.empty[Row] // drop local var
)
verifySqlScriptResult(sqlScript, expected)
}
}

test("for statement - nested empty body") {
withTable("t") {
val sqlScript =
"""
|BEGIN
| CREATE TABLE t (intCol INT, stringCol STRING, doubleCol DOUBLE) using parquet;
| INSERT INTO t VALUES (1, 'first', 1.0);
| FOR row AS SELECT * FROM t DO
| BEGIN
| BEGIN
| END;
| END;
| BEGIN
| BEGIN
| END;
| END;
| END FOR;
|END
|""".stripMargin

val expected = Seq(
Seq.empty[Row], // create table
Seq.empty[Row], // insert
Seq.empty[Row], // drop local var
Seq.empty[Row], // drop local var
Seq.empty[Row], // drop local var
Seq.empty[Row] // drop local var
)
verifySqlScriptResult(sqlScript, expected)
}
}

test("for statement iterate") {
withTable("t") {
val sqlScript =
Expand Down

0 comments on commit e70f8ab

Please sign in to comment.