cloud-fan commented on code in PR #58530:
URL: https://github.com/apache/spark/pull/58530#discussion_r3979010260
##########
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
+ * linear boundary parse instead.
*/
object SqlStatementSplitter {
/** Split the given SQL text into individual statements at `;` boundaries. */
def split(sqlText: String): SqlStatementSplitResult =
- split(sqlText, identity)
+ splitWithPositions(sqlText, identity).withoutPositions
+
+ /**
+ * Split a parse_sql batch in one grammar-owned pass while retaining source
positions.
+ * Unlike the generic splitter, this boundary-only grammar accepts malformed
leaf statements
+ * and uses scripting grammar contexts to assign internal semicolons to
compound statements.
+ */
+ private[sql] def splitForParseSql(sqlText: String):
PositionedSqlStatementSplitResult = {
+ require(sqlText != null, "sqlText must not be null")
+
+ val toUtf16 = utf16Offsets(sqlText)
+ val sourceLexer = new SqlBaseLexer(new
UpperCaseCharStream(CharStreams.fromString(sqlText)))
+ sourceLexer.removeErrorListeners()
+ val sourceTokens = new CommonTokenStream(sourceLexer)
+ sourceTokens.fill()
+ val boundaryTokens = new java.util.ArrayList[Token](sourceTokens.size() +
1)
+ var sourceIndex = 0
+ while (sourceIndex < sourceTokens.size() - 1) {
+ boundaryTokens.add(sourceTokens.get(sourceIndex))
+ sourceIndex += 1
+ }
+ val boundary = new CommonToken(SqlBaseParser.PARSE_SQL_BATCH_DELIMITER, "")
+ boundary.setStartIndex(toUtf16.length - 1)
+ boundary.setStopIndex(toUtf16.length - 2)
+ boundaryTokens.add(boundary)
+ boundaryTokens.add(sourceTokens.get(sourceTokens.size() - 1))
+ val tokens = new CommonTokenStream(new ListTokenSource(boundaryTokens))
+ tokens.fill()
+ val parser = new SqlBaseParser(tokens)
+ configureSplitterParser(parser, SqlApiConf.get)
+ parser.getInterpreter.setPredictionMode(PredictionMode.LL)
+ val batch = try {
+ parser.parseSqlBatch()
Review Comment:
**Blocking (P1):** `BailErrorStrategy` can throw
`ParseCancellationException` here, but this path catches only
`StackOverflowError`. For `BEGIN BEGIN; END; END; SELECT 3`, the exception
escapes before parse_sql can return either the malformed statement error or the
later successful SELECT result. Please make the boundary parse total for
malformed token streams, containing cancellation through a recovery path that
still preserves top-level suffix boundaries, and add a no-throw regression for
this input.
**Recommended change:** Contain parser cancellation at the parse_sql
boundary and recover statement boundaries without reverting to a suffix-merging
fallback.
**Why this works:** Use a boundary-parser error strategy or a narrowly
handled ParseCancellationException path that returns context-owned malformed
items and then resumes with later top-level statements.
**Scope:** A focused SqlStatementSplitter parser-configuration or
exception-handling change with one malformed nested-BEGIN regression.
**Compatibility:** Preserve propagation of unexpected internal failures,
generic splitter behavior, and all successful boundary parses; convert only
expected boundary-parser cancellation into per-statement parse_sql results.
**Risks:** A broad catch could hide unrelated internal parser failures.
Falling back to the generic or partial path without structural checks could
reintroduce suffix merging.
**Constraints:** Handle only the boundary parser's expected cancellation
mode. Keep unexpected internal failures propagating as documented. Return later
valid statements independently after recovery.
**Success:** The nested-BEGIN counterexample no longer throws and returns a
failed malformed item plus the separate successful SELECT 3 item, with existing
valid and malformed boundary cases unchanged.
##########
sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4:
##########
@@ -87,6 +89,97 @@ 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
+ | parseSqlBatchLeafStatement
+ ;
+
+parseSqlBatchPartialCompoundStatement
+ : BEGIN .*?
Review Comment:
**Blocking (P1):** This fallback treats every BEGIN-led suffix as an
incomplete compound until the synthetic batch delimiter. For `BEGIN; SELECT
1;`, it emits one failed item for the whole input, so the valid SELECT result
disappears; balanced malformed blocks have the same suffix-capture failure.
Please distinguish a definitively malformed BEGIN statement from a genuinely
incomplete compound, retain a balanced block through its structural outer
boundary, and then resume the batch. Add parse_sql-level regressions that
require the malformed item and the following valid statement to remain separate.
**Recommended change:** Make the grammar-owned fallback distinguish
semicolon-terminated malformed BEGIN input from genuinely incomplete or
structurally balanced compound input.
**Why this works:** Constrain or split the partial-compound recovery so it
emits a definitively malformed item at its own top-level boundary, preserves a
balanced malformed block through its matching outer END, and resumes
parseSqlBatch for the suffix.
**Scope:** A focused batch-boundary grammar change with matching
SqlStatementSplitterSuite and ParseSqlResultSuite regressions.
**Compatibility:** Keep generic splitter behavior, valid nested BEGIN ...
END grouping, source spans, and ordinary wildcard statement handling unchanged;
alter only malformed parse_sql boundary recovery.
**Risks:** An over-eager semicolon boundary could split a valid nested
script or wildcard command body. A permissive fallback could preserve the
current suffix-merging behavior for another malformed compound shape.
**Constraints:** Use grammar-owned structural delimiters rather than
reconstructing scripting depth from raw tokens. Do not discard or merge later
top-level statements after a malformed item. Preserve the existing UTF-16 span
contract.
**Success:** BEGIN; SELECT 1; and representative balanced malformed
compounds each produce one failed item followed by the separate successful
SELECT item, while valid compound and generic-splitter tests remain unchanged.
##########
sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4:
##########
@@ -87,6 +89,97 @@ 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
+ | parseSqlBatchLeafStatement
+ ;
+
+parseSqlBatchPartialCompoundStatement
+ : BEGIN .*?
+ ;
+
+parseSqlBatchCompoundStatement
+ : BEGIN (NOT ATOMIC)? parseSqlBatchCompoundBody? END
+ ;
+
+parseSqlBatchBeginEndCompoundBlock
+ : beginLabel? BEGIN (NOT ATOMIC)? parseSqlBatchCompoundBody? END endLabel?
+ ;
+
+parseSqlBatchCompoundBody
+ : (parseSqlBatchCompoundBodyStatement SEMICOLON)+
+ ;
+
+parseSqlBatchCompoundBodyStatement
+ : parseSqlBatchBeginEndCompoundBlock
+ | parseSqlBatchDeclareHandlerStatement
+ | parseSqlBatchIfElseStatement
+ | parseSqlBatchCaseStatement
+ | parseSqlBatchWhileStatement
+ | parseSqlBatchRepeatStatement
+ | parseSqlBatchLoopStatement
+ | parseSqlBatchForStatement
+ | parseSqlBatchLeafStatement
+ ;
+
+parseSqlBatchDeclareHandlerStatement
+ : DECLARE (CONTINUE | EXIT) HANDLER FOR conditionValues
+ (parseSqlBatchBeginEndCompoundBlock | parseSqlBatchLeafStatement)
+ ;
+
+parseSqlBatchWhileStatement
+ : beginLabel? WHILE booleanExpression DO parseSqlBatchCompoundBody END
WHILE endLabel?
+ ;
+
+parseSqlBatchIfElseStatement
+ : IF booleanExpression THEN parseSqlBatchCompoundBody
+ (ELSEIF booleanExpression THEN parseSqlBatchCompoundBody)*
+ (ELSE parseSqlBatchCompoundBody)? END IF
+ ;
+
+parseSqlBatchRepeatStatement
+ : beginLabel? REPEAT parseSqlBatchCompoundBody UNTIL booleanExpression END
REPEAT endLabel?
+ ;
+
+parseSqlBatchCaseStatement
+ : CASE (WHEN booleanExpression THEN parseSqlBatchCompoundBody)+
+ (ELSE parseSqlBatchCompoundBody)? END CASE
+ | CASE expression (WHEN expression THEN parseSqlBatchCompoundBody)+
+ (ELSE parseSqlBatchCompoundBody)? END CASE
+ ;
+
+parseSqlBatchLoopStatement
+ : beginLabel? LOOP parseSqlBatchCompoundBody END LOOP endLabel?
+ ;
+
+parseSqlBatchForStatement
+ : beginLabel? FOR (strictIdentifier AS)? query DO
+ parseSqlBatchCompoundBody END FOR endLabel?
+ ;
+
+parseSqlBatchLeafStatement
+ : {_input.LA(1) != BEGIN}?
+ (~(SEMICOLON | PARSE_SQL_BATCH_DELIMITER))+
Review Comment:
**Blocking (P1):** A body leaf can start with `END`, so the greedy body loop
consumes the valid enclosing END and closes the block only at a later END. With
`BEGIN SELECT 1; END; END; SELECT 2`, the first valid compound and the stray
END become one failed item instead of separate top-level statements. Please
make the compound-body boundary reserve its structural END for the enclosing
rule and add splitter/result regressions that prove the first END closes the
valid block.
**Recommended change:** Make END ownership explicit in the compound-body
grammar so a body leaf cannot consume the enclosing terminator.
**Why this works:** Constrain the leaf alternative in compound-body position
so a structural END is matched by the enclosing compound rule, while ordinary
leaf parsing outside that position remains unchanged.
**Scope:** A small grammar correction plus focused splitter and parse_sql
result regressions.
**Compatibility:** Preserve valid leaf statements, labels, nested control
constructs, generic splitting, and source positions; only correct ownership of
a compound's structural END.
**Risks:** A blanket END exclusion in the wrong grammar context could reject
an otherwise supported identifier use.
**Constraints:** Apply the restriction only where END is the enclosing
compound delimiter. Keep nested BEGIN and control-statement terminators
grammar-owned.
**Success:** The counterexample yields the valid compound, the stray END
error, and SELECT 2 as three ordered results, and existing valid
compound-script cases remain one item.
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/parser/ParseSqlResult.scala:
##########
@@ -43,12 +43,18 @@ import
org.apache.spark.sql.execution.datasources.CreateTempViewUsing
* executors without a session, so only the stock parser is available under
* distributed eval.
*
- * On success the JSON always includes `parse_success`, the statement
+ * Every statement object includes its 1-based UTF-16 code-unit `start` in the
+ * original batch and its UTF-16 code-unit `length`, excluding surrounding
+ * whitespace and the terminating semicolon. On success it also includes
+ * `parse_success`, the statement
* identifier/code (ISO/IEC 9075-2:2023 Table 39), and omits unused optional
* fields (`target_table_references`, `source_table_references`,
- * `function_references`, `select_list`, `parameter_markers`) when empty. On
parse
- * failure it returns `parse_success: false` with source location and a nested
- * STANDARD-format error object, and does not throw. Only [[ParseException]] /
+ * `function_references`, `select_list`, `parameter_markers`) when empty. On
+ * parse failure the statement object contains `parse_success: false` with
+ * source location and a nested STANDARD-format error object, and parsing
+ * continues with later statements. Nested error locations are relative to the
+ * individual statement, while `start` is relative to the original batch. An
+ * empty or comment-only batch produces an empty array. Only
[[ParseException]] /
Review Comment:
**Non-blocking (P2):** This guarantee does not hold for an unterminated
block comment: `/* unclosed` is preserved as a partial statement and returned
as a `parse_success:false` object. Please qualify the claim to empty or
closed-comment-only batches, or change the behavior if every comment-only input
is intended to return `[]`.
--
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]
