[SPARK-48348][SPARK-48376][SQL] Introduce `LEAVE` and `ITERATE` statements #47973

davidm-db · 2024-09-03T10:36:22Z

What changes were proposed in this pull request?

This PR proposes introduction of LEAVE and ITERATE statement types to SQL Scripting language:

LEAVE statement can be used in loops, as well as in BEGIN ... END compound blocks.
ITERATE statement can be used only in loops.

This PR introduces:

Logical operators for both statement types.
Execution nodes for both statement types.
Interpreter changes required to build execution plans that support new statement types.
New error if statements are not used properly.
Minor changes required to support new keywords.

Why are the changes needed?

Adding support for new statement types to SQL Scripting language.

Does this PR introduce any user-facing change?

This PR introduces new statement types that will be available to users. However, script execution logic hasn't been done yet, so the new changes are not accessible by users yet.

How was this patch tested?

Tests are introduced to all test suites related to SQL scripting.

Was this patch authored or co-authored using generative AI tooling?

No.

davidm-db · 2024-09-03T15:09:08Z

sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionNode.scala

+      // Stop the iteration.
+      stopIteration = true
+
+      // TODO: Variable cleanup (once we add SQL script execution logic).


Will be done once we introduce execution. For now, we are adding DropVariable statements at the end of each block implicitly. However, introduction of LEAVE and ITERATE statements means that these implicit statements will be skipped and we need to add logic to execute them immediately. Currently, we cannot do so, because execution nodes don't have execution logic yet.

cloud-fan · 2024-09-04T13:53:37Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

+      ctx: RuleContext, label: String, isLeave: Boolean): Boolean = {
+    ctx match {
+      case c: BeginEndCompoundBlockContext
+        if isLeave &&


shall we have a dedicated error for using ITERATE on the label of BEGIN END compound

good idea, will do

sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionNode.scala

cloud-fan · 2024-09-04T13:59:21Z

sql/core/src/test/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionNodeSuite.scala

@@ -35,6 +35,10 @@ class SqlScriptingExecutionNodeSuite extends SparkFunSuite with SharedSparkSessi
    override def reset(): Unit = ()
  }

+  case class TestLeaveStatement(labelText: String) extends LeaveStatementExec(labelText)
+
+  case class TestIterateStatement(labelText: String) extends IterateStatementExec(labelText)


Why can't we use the real Leave/IterateStatementExec in this test?

because I'm dumb :) Used these in extractStatementValue to match on labelText directly instead of matching by type only and using Leave/IterateStatementExec... will change in the next commit to use regular exec nodes

dongjoon-hyun

+1, LGTM.

cc @yaooqinn , too.

yaooqinn · 2024-09-05T03:06:31Z

common/utils/src/main/resources/error/error-conditions.json

@@ -2453,6 +2453,12 @@
    },
    "sqlState" : "42K0K"
  },
+  "INVALID_ITERATE_LABEL_USAGE_FOR_COMPOUND" : {


Can these error classes be put together with subclasses? like
INVALID_LABEL.ITERATE_FOR_COMPOUND / INVALID_LABEL.IN_STATEMENT

yaooqinn

LGTM

miland-db · 2024-09-05T10:31:55Z

sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionNode.scala

+            // Handle LEAVE or ITERATE statement if it has been encountered.
+            retStmt match {
+              case leaveStatementExec: LeaveStatementExec if !leaveStatementExec.hasBeenMatched =>
+                if (label.contains(leaveStatementExec.label)) {


Is there a reason why we use contains instead of equals?

it's not contains like in Strings, it's from the perspective of Option, i.e. it means whether the value within the Option is leaveStatementExec.label

miland-db · 2024-09-05T10:35:24Z

LGTM

davidm-db · 2024-09-05T11:36:41Z

@cloud-fan @yaooqinn @dongjoon-hyun can someone merge this now, all tests are fine? also, could you please close both JIRA items, because it seems that only the first one from the title gets linked to the PR.

cloud-fan · 2024-09-05T12:58:49Z

thanks, merging to master!

…nSqlScript in SQL Scripting Interpreter test suite ### What changes were proposed in this pull request? Previous [pull request](#47973) introduced new tests to `SqlScriptingInterpreterSuite` (among others) where accidentally `parseScript` was used instead of `runSqlScript`. While the same exception would get thrown (since it happens in the parsing phase) it violates the consistency among the tests in this suite and adds unnecessary import, so it would be nice to change it. ### Why are the changes needed? Changes are minor, they improve consistency among test suites for SQL scripting. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? This patch alters tests. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #48016 from davidm-db/interpreter_test_suite_fix. Authored-by: David Milicevic <david.milicevic@databricks.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

…nSqlScript in SQL Scripting Interpreter test suite ### What changes were proposed in this pull request? Previous [pull request](apache/spark#47973) introduced new tests to `SqlScriptingInterpreterSuite` (among others) where accidentally `parseScript` was used instead of `runSqlScript`. While the same exception would get thrown (since it happens in the parsing phase) it violates the consistency among the tests in this suite and adds unnecessary import, so it would be nice to change it. ### Why are the changes needed? Changes are minor, they improve consistency among test suites for SQL scripting. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? This patch alters tests. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #48016 from davidm-db/interpreter_test_suite_fix. Authored-by: David Milicevic <david.milicevic@databricks.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

### What changes were proposed in this pull request? Previous [pull request](#47973) introduced new logic for `LEAVE` and `ITERATE` statements, but I missed to introduce minor part of `ITERATE` statement handling. While `ITERATE` statement is not allowed for compounds (i.e. user cannot use `LEAVE <label>` where label belongs to compound) it's still possible to use `ITERATE` statement within the compound (i.e. when using label of the surrounding `WHILE` loop for example). In such cases, it's required to drop variables defined in the compound (the same way as for `ITERATE` statement). We don't yet have execution logic (I will work on POC and design doc during the next week), but this is setting up stuff now, so we don't miss anything in future. ### Why are the changes needed? Changes are minor, they improve consistency among test suites for SQL scripting. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added small test, but not completely related to this change. Since we don't have execution logic yet, it's hard to test. I will add more tests together with the execution logic. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #48022 from davidm-db/missing_logic_for_leave_statement. Authored-by: David Milicevic <david.milicevic@databricks.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

…ments ### What changes were proposed in this pull request? This PR proposes introduction of `LEAVE` and `ITERATE` statement types to SQL Scripting language: - `LEAVE` statement can be used in loops, as well as in `BEGIN ... END` compound blocks. - `ITERATE` statement can be used only in loops. This PR introduces: - Logical operators for both statement types. - Execution nodes for both statement types. - Interpreter changes required to build execution plans that support new statement types. - New error if statements are not used properly. - Minor changes required to support new keywords. ### Why are the changes needed? Adding support for new statement types to SQL Scripting language. ### Does this PR introduce _any_ user-facing change? This PR introduces new statement types that will be available to users. However, script execution logic hasn't been done yet, so the new changes are not accessible by users yet. ### How was this patch tested? Tests are introduced to all test suites related to SQL scripting. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47973 from davidm-db/sql_scripting_leave_iterate. Authored-by: David Milicevic <david.milicevic@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…nSqlScript in SQL Scripting Interpreter test suite ### What changes were proposed in this pull request? Previous [pull request](apache#47973) introduced new tests to `SqlScriptingInterpreterSuite` (among others) where accidentally `parseScript` was used instead of `runSqlScript`. While the same exception would get thrown (since it happens in the parsing phase) it violates the consistency among the tests in this suite and adds unnecessary import, so it would be nice to change it. ### Why are the changes needed? Changes are minor, they improve consistency among test suites for SQL scripting. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? This patch alters tests. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#48016 from davidm-db/interpreter_test_suite_fix. Authored-by: David Milicevic <david.milicevic@databricks.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

### What changes were proposed in this pull request? Previous [pull request](apache#47973) introduced new logic for `LEAVE` and `ITERATE` statements, but I missed to introduce minor part of `ITERATE` statement handling. While `ITERATE` statement is not allowed for compounds (i.e. user cannot use `LEAVE <label>` where label belongs to compound) it's still possible to use `ITERATE` statement within the compound (i.e. when using label of the surrounding `WHILE` loop for example). In such cases, it's required to drop variables defined in the compound (the same way as for `ITERATE` statement). We don't yet have execution logic (I will work on POC and design doc during the next week), but this is setting up stuff now, so we don't miss anything in future. ### Why are the changes needed? Changes are minor, they improve consistency among test suites for SQL scripting. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added small test, but not completely related to this change. Since we don't have execution logic yet, it's hard to test. I will add more tests together with the execution logic. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#48022 from davidm-db/missing_logic_for_leave_statement. Authored-by: David Milicevic <david.milicevic@databricks.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

…ments ### What changes were proposed in this pull request? This PR proposes introduction of `LEAVE` and `ITERATE` statement types to SQL Scripting language: - `LEAVE` statement can be used in loops, as well as in `BEGIN ... END` compound blocks. - `ITERATE` statement can be used only in loops. This PR introduces: - Logical operators for both statement types. - Execution nodes for both statement types. - Interpreter changes required to build execution plans that support new statement types. - New error if statements are not used properly. - Minor changes required to support new keywords. ### Why are the changes needed? Adding support for new statement types to SQL Scripting language. ### Does this PR introduce _any_ user-facing change? This PR introduces new statement types that will be available to users. However, script execution logic hasn't been done yet, so the new changes are not accessible by users yet. ### How was this patch tested? Tests are introduced to all test suites related to SQL scripting. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47973 from davidm-db/sql_scripting_leave_iterate. Authored-by: David Milicevic <david.milicevic@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…nSqlScript in SQL Scripting Interpreter test suite ### What changes were proposed in this pull request? Previous [pull request](apache#47973) introduced new tests to `SqlScriptingInterpreterSuite` (among others) where accidentally `parseScript` was used instead of `runSqlScript`. While the same exception would get thrown (since it happens in the parsing phase) it violates the consistency among the tests in this suite and adds unnecessary import, so it would be nice to change it. ### Why are the changes needed? Changes are minor, they improve consistency among test suites for SQL scripting. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? This patch alters tests. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#48016 from davidm-db/interpreter_test_suite_fix. Authored-by: David Milicevic <david.milicevic@databricks.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

### What changes were proposed in this pull request? Previous [pull request](apache#47973) introduced new logic for `LEAVE` and `ITERATE` statements, but I missed to introduce minor part of `ITERATE` statement handling. While `ITERATE` statement is not allowed for compounds (i.e. user cannot use `LEAVE <label>` where label belongs to compound) it's still possible to use `ITERATE` statement within the compound (i.e. when using label of the surrounding `WHILE` loop for example). In such cases, it's required to drop variables defined in the compound (the same way as for `ITERATE` statement). We don't yet have execution logic (I will work on POC and design doc during the next week), but this is setting up stuff now, so we don't miss anything in future. ### Why are the changes needed? Changes are minor, they improve consistency among test suites for SQL scripting. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added small test, but not completely related to this change. Since we don't have execution logic yet, it's hard to test. I will add more tests together with the execution logic. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#48022 from davidm-db/missing_logic_for_leave_statement. Authored-by: David Milicevic <david.milicevic@databricks.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

github-actions bot added SQL DOCS labels Sep 3, 2024

davidm-db commented Sep 3, 2024

View reviewed changes

davidm-db added 5 commits September 4, 2024 11:06

Introduce LEAVE and ITERATE statements

78ba03d

Add missing stuff for new keywords

32bfb68

Add proper error messages

61d4237

Fix accidental edit

3a09416

Add tests

b5b6b25

davidm-db force-pushed the sql_scripting_leave_iterate branch from c9b8372 to b5b6b25 Compare September 4, 2024 09:06

cloud-fan reviewed Sep 4, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionNode.scala Show resolved Hide resolved

cloud-fan reviewed Sep 4, 2024

View reviewed changes

cloud-fan approved these changes Sep 4, 2024

View reviewed changes

davidm-db added 3 commits September 4, 2024 20:39

Remove test exec nodes for LEAVE and ITERATE statements

0e8fa23

Add error class for usage of ITERATE statement for compound blocks

22e49fa

Add missing comments

23bb413

dongjoon-hyun changed the title ~~[SPARK-48348][SPARK-48376][SQL] Introduce LEAVE and ITERATE statements~~ [SPARK-48348][SPARK-48376][SQL] Introduce LEAVE and ITERATE statements Sep 4, 2024

dongjoon-hyun approved these changes Sep 4, 2024

View reviewed changes

yaooqinn reviewed Sep 5, 2024

View reviewed changes

Group error classes

ca98632

yaooqinn approved these changes Sep 5, 2024

View reviewed changes

miland-db reviewed Sep 5, 2024

View reviewed changes

miland-db approved these changes Sep 5, 2024

View reviewed changes

cloud-fan closed this in 9676b1c Sep 5, 2024

davidm-db mentioned this pull request Sep 6, 2024

[SPARK-48348][SPARK-48376][FOLLOWUP][SQL] Replace parseScript with runSqlScript in SQL Scripting Interpreter test suite #48016

Closed

davidm-db mentioned this pull request Sep 7, 2024

[SPARK-48376][SQL][FOLLOWUP] Add ITERATE statement fixes #48022

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-48348][SPARK-48376][SQL] Introduce `LEAVE` and `ITERATE` statements #47973

[SPARK-48348][SPARK-48376][SQL] Introduce `LEAVE` and `ITERATE` statements #47973

davidm-db commented Sep 3, 2024 •

edited

Loading

davidm-db Sep 3, 2024

cloud-fan Sep 4, 2024

davidm-db Sep 4, 2024

davidm-db Sep 4, 2024

cloud-fan Sep 4, 2024

davidm-db Sep 4, 2024

davidm-db Sep 4, 2024

dongjoon-hyun left a comment

yaooqinn Sep 5, 2024

davidm-db Sep 5, 2024

yaooqinn left a comment

miland-db Sep 5, 2024

davidm-db Sep 5, 2024

miland-db commented Sep 5, 2024

davidm-db commented Sep 5, 2024

cloud-fan commented Sep 5, 2024

[SPARK-48348][SPARK-48376][SQL] Introduce LEAVE and ITERATE statements #47973

[SPARK-48348][SPARK-48376][SQL] Introduce LEAVE and ITERATE statements #47973

Conversation

davidm-db commented Sep 3, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaooqinn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miland-db commented Sep 5, 2024

davidm-db commented Sep 5, 2024

cloud-fan commented Sep 5, 2024

[SPARK-48348][SPARK-48376][SQL] Introduce `LEAVE` and `ITERATE` statements #47973

[SPARK-48348][SPARK-48376][SQL] Introduce `LEAVE` and `ITERATE` statements #47973

davidm-db commented Sep 3, 2024 •

edited

Loading