cloud-fan commented on code in PR #58530:
URL: https://github.com/apache/spark/pull/58530#discussion_r3991356403
##########
sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4:
##########
@@ -87,6 +89,181 @@ compoundOrSingleStatement
| singleCompoundStatement
;
+// Boundary-only grammar for parse_sql batches. Leaf statements deliberately
accept arbitrary
+// tokens: ParseSqlResult parses each emitted segment with the full grammar
and records any error.
+// BEGIN is excluded from the terminated fallback, so only a grammar context
can own the
+// semicolons inside a compound statement. BEGIN and END remain unrestricted
inside leaf
+// statements. The caller appends PARSE_SQL_BATCH_DELIMITER; the trailing
BEGIN fallback consumes
+// a structurally unclosed compound through that token without synthesizing an
END token.
+parseSqlBatch
+ : SEMICOLON* (items+=parseSqlBatchItem SEMICOLON*)*
+ PARSE_SQL_BATCH_DELIMITER? EOF
+ ;
+
+parseSqlBatchItem
+ : batchStatement=parseSqlBatchStatement
+ terminator=(SEMICOLON | PARSE_SQL_BATCH_DELIMITER)
+ | partialStatement=parseSqlBatchPartialCompoundStatement
+ terminator=PARSE_SQL_BATCH_DELIMITER
+ ;
+
+parseSqlBatchStatement
+ : parseSqlBatchCompoundStatement
+ | parseSqlBatchMalformedEmptyCompoundBlock
+ | parseSqlBatchMalformedBeginStatement
+ | parseSqlBatchLeafStatement
+ ;
+
+parseSqlBatchMalformedBeginStatement
+ : BEGIN
+ ;
+
+parseSqlBatchPartialCompoundStatement
+ : BEGIN .*?
Review Comment:
**Blocking (P1):** This catch-all can terminate only at the synthetic batch
delimiter, so a balanced but malformed compound that misses the precise
alternatives absorbs every later statement. For example, `BEGIN SELECT 1; END
bad; SELECT 2` and `BEGIN SELECT 1 END; SELECT 2` each become one failed
segment, and the valid `SELECT 2` result disappears. Please recover a balanced
or semicolon-delimited malformed BEGIN item at its own top-level boundary,
reserving whole-remainder consumption for a genuinely unmatched trailing
compound.
**Recommended change:** Replace the unconditional BEGIN-to-end-of-batch
catch-all with recovery alternatives that terminate a balanced malformed BEGIN
item at its own top-level delimiter and reserve whole-remainder consumption for
a genuinely unmatched trailing compound.
**Why this works:** Track structural nesting while recovering a BEGIN-led
item. Once the matching top-level compound closer has been reached, let
malformed suffix tokens belong to that same statement only through its
terminating semicolon, then resume parseSqlBatch at the next token. If no
matching closer or safe top-level delimiter exists, retain the end-of-batch
fallback for the incomplete trailing item.
**Scope:** sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser,
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser,
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser,
sql/core/src/test/scala/org/apache/spark/sql/catalyst/parser
**Compatibility:** A malformed compound remains one result, but it no longer
owns unrelated top-level statements that begin after its structural or
delimiter boundary.
**Risks:** Recovery that accepts the first END token without nesting
ownership could confuse inner and outer compound closers. Recovery that stops
at an internal semicolon could regress the existing guarantee that a malformed
compound body remains one result.
**Constraints:** Do not use raw-token heuristics that lose nested
control-flow ownership or reintroduce growing-prefix parsing on the ordinary
path. Preserve the existing BEGIN;, bare END, premature END, malformed nested
BEGIN, and malformed-body behavior already covered by tests. Continue to
represent a truly unterminated final compound as one trailing failed statement.
**Success:** BEGIN SELECT 1; END bad; SELECT 2 produces a failed first
result and a successful SELECT 2 result with separate spans. BEGIN SELECT 1
END; SELECT 2 likewise isolates the malformed compound and continues with
SELECT 2. Nested valid or malformed compounds keep their internal semicolons
and structural END tokens within the correct outer result.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/SqlStatementSplitter.scala:
##########
@@ -97,21 +117,116 @@ case class SqlStatementSplitResult(
* for real at execution time. When `validationPreprocess` is `identity`
* (the default), the splitter behaves as a pure original-text splitter.
*
- * Performance note: for a single `BEGIN ... END` block with k internal `;`,
- * the splitter calls `tryParseRegion` O(k) times on growing prefixes -- an
+ * Performance note: the generic splitter calls `tryParseRegion` O(k) times on
+ * growing prefixes for a single `BEGIN ... END` block with k internal `;` --
an
* O(k^2) cost in the worst case (incomplete block on every keystroke in
* interactive mode). Ordinary non-scripting SQL is O(n). A non-EOF terminated
* single-statement rule (read `ctx.getStop` once per region) would make this
* O(n), but Spark's `setResetStatement` has `SET .*?` / `RESET .*?` wildcards
* that need an EOF anchor to terminate deterministically, so such a
* single-statement rule-rewrite does not drop in cleanly. Tracked as a
- * follow-up.
+ * follow-up. The parse_sql-only path uses [[splitForParseSql]] and performs
one
Review Comment:
**Nit (P3):** The normal parse_sql path does perform one boundary parse, but
the `StackOverflowError` branch below falls back to `splitWithPositions`, whose
compound-input worst case is documented here as O(k^2). Could this qualify the
claim as applying to the normal path and mention that exceptional fallback?
##########
sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4:
##########
@@ -87,6 +89,181 @@ compoundOrSingleStatement
| singleCompoundStatement
;
+// Boundary-only grammar for parse_sql batches. Leaf statements deliberately
accept arbitrary
+// tokens: ParseSqlResult parses each emitted segment with the full grammar
and records any error.
+// BEGIN is excluded from the terminated fallback, so only a grammar context
can own the
+// semicolons inside a compound statement. BEGIN and END remain unrestricted
inside leaf
+// statements. The caller appends PARSE_SQL_BATCH_DELIMITER; the trailing
BEGIN fallback consumes
+// a structurally unclosed compound through that token without synthesizing an
END token.
+parseSqlBatch
+ : SEMICOLON* (items+=parseSqlBatchItem SEMICOLON*)*
+ PARSE_SQL_BATCH_DELIMITER? EOF
+ ;
+
+parseSqlBatchItem
+ : batchStatement=parseSqlBatchStatement
+ terminator=(SEMICOLON | PARSE_SQL_BATCH_DELIMITER)
+ | partialStatement=parseSqlBatchPartialCompoundStatement
+ terminator=PARSE_SQL_BATCH_DELIMITER
+ ;
+
+parseSqlBatchStatement
+ : parseSqlBatchCompoundStatement
+ | parseSqlBatchMalformedEmptyCompoundBlock
+ | parseSqlBatchMalformedBeginStatement
+ | parseSqlBatchLeafStatement
+ ;
+
+parseSqlBatchMalformedBeginStatement
+ : BEGIN
+ ;
+
+parseSqlBatchPartialCompoundStatement
+ : BEGIN .*?
+ ;
+
+parseSqlBatchCompoundStatement
+ : BEGIN (NOT ATOMIC)? parseSqlBatchCompoundBody? END
+ ;
+
+parseSqlBatchBeginEndCompoundBlock
+ : beginLabel? BEGIN (NOT ATOMIC)? parseSqlBatchCompoundBody? END endLabel?
+ ;
+
+parseSqlBatchMalformedEmptyCompoundBlock
+ : beginLabel? BEGIN (NOT ATOMIC)? SEMICOLON END endLabel?
+ ;
+
+parseSqlBatchMalformedBodyBeginStatement
+ : BEGIN (~(SEMICOLON | PARSE_SQL_BATCH_DELIMITER))+
+ ;
+
+parseSqlBatchCompoundBody
+ : (parseSqlBatchCompoundBodyStatement SEMICOLON)+
+ ;
+
+parseSqlBatchCompoundBodyStatement
+ : parseSqlBatchNestedStatement
+ | parseSqlBatchOrphanControlEndStatement
+ | parseSqlBatchBodyLeafStatement
+ ;
+
+parseSqlBatchNestedStatement
+ : parseSqlBatchBeginEndCompoundBlock
+ | parseSqlBatchMalformedEmptyCompoundBlock
+ | parseSqlBatchDeclareHandlerStatement
+ | parseSqlBatchIfElseStatement
+ | parseSqlBatchCaseStatement
+ | parseSqlBatchWhileStatement
+ | parseSqlBatchRepeatStatement
+ | parseSqlBatchLoopStatement
+ | parseSqlBatchForStatement
+ | parseSqlBatchMalformedBodyBeginStatement
+ | parseSqlBatchMalformedBeginStatement
+ ;
+
+parseSqlBatchOrphanControlEndStatement
+ : END (IF | WHILE | LOOP | REPEAT | FOR | CASE)
+ ;
+
+parseSqlBatchDeclareHandlerStatement
+ : DECLARE (CONTINUE | EXIT) HANDLER FOR conditionValues
+ (parseSqlBatchBeginEndCompoundBlock
+ | parseSqlBatchMalformedEmptyCompoundBlock
+ | parseSqlBatchMalformedBodyBeginStatement
+ | parseSqlBatchBodyLeafStatement)
+ ;
+
+parseSqlBatchWhileStatement
+ : beginLabel? WHILE booleanExpression DO parseSqlBatchCompoundBody
Review Comment:
**Blocking (P1):** This boundary rule parses the raw batch before
SparkSqlParser applies `${...}` substitution. Unlike the delimiter-bounded
IF/CASE header rules, WHILE requires a complete `booleanExpression`; REPEAT has
the same issue for `UNTIL`, and FOR requires a complete `query`. A valid script
such as `BEGIN WHILE ${flag} DO BEGIN SELECT 1; END; END WHILE; END; SELECT 2`
can therefore fall through to leaf ownership, misassign nested semicolons/ENDs,
and lose the correct top-level result boundaries. Please make these loop
headers substitution-tolerant during boundary discovery while preserving the
original text for stock parsing and source spans.
**Recommended change:** Make WHILE, REPEAT, and FOR boundary rules tolerate
raw variable-reference tokens by parsing their header regions as
delimiter-bounded structural content rather than requiring a complete
booleanExpression or query during boundary discovery.
**Why this works:** Introduce permissive boundary-only header subrules,
analogous to the existing IF and CASE boundary rules, that consume header
tokens up to the construct's structural delimiter while leaving the original
text and UTF-16 spans unchanged. Continue sending each resulting original-text
segment to SparkSqlParser so configured substitution and semantic validation
happen exactly once at the established owner.
**Scope:** sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser,
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser,
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser,
sql/core/src/test/scala/org/apache/spark/sql/catalyst/parser
**Compatibility:** The stock SparkSqlParser remains solely responsible for
applying configured variable substitution and deciding whether each original
statement parses.
**Risks:** An over-permissive header rule could stop at a delimiter-like
keyword that is nested inside a parenthesized expression or subquery. A change
to structural ownership could alter boundaries for malformed loop statements
even when no variable reference is present.
**Constraints:** Preserve original statement text and 1-based UTF-16 start
and length values. Do not change the generic splitStatements path or move
semantic validation out of SparkSqlParser. Keep nested BEGIN, IF, CASE, LOOP,
WHILE, REPEAT, and FOR bodies owned by their enclosing top-level statement.
**Success:** With variable substitution enabled, raw ${...} references in
WHILE conditions, REPEAT UNTIL conditions, and FOR queries do not alter
top-level segmentation before stock parsing. A valid substituted loop script
followed by SELECT 2 yields the loop script and SELECT 2 as two ordered results
with correct original-text spans. Malformed loop headers still produce
per-statement parse errors without swallowing unrelated later top-level
statements.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
