cloud-fan commented on code in PR #58530:
URL: https://github.com/apache/spark/pull/58530#discussion_r3971952378
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/SqlStatementSplitter.scala:
##########
@@ -285,8 +370,155 @@ object SqlStatementSplitter {
val unclosed = lexer.has_unclosed_bracketed_comment
val partial =
- if (bufferHasContent || unclosed) buffer.toString.trim else ""
- SqlStatementSplitResult(completeStatements.toSeq, partial, unclosed &&
partial.nonEmpty)
+ if (bufferHasContent || unclosed) positionedStatement("") else None
+ PositionedSqlStatementSplitResult(
+ completeStatements.toSeq,
+ partial,
+ unclosed && partial.nonEmpty)
+ }
+
+ /**
+ * Returns the delimiter-array index and ending token index of a real outer
END for a malformed
+ * compound statement. Error recovery may repair the body, but a missing END
is synthetic and
+ * has token index -1.
+ */
+ private def findMalformedCompoundEnd(
+ sqlText: String,
+ toUtf16: Array[Int],
+ stream: CommonTokenStream,
+ startIdx: Int,
+ delimiterPositions: Array[Int],
+ fromDelimiter: Int,
+ validationPreprocess: String => String,
+ conf: SqlApiConf): Option[(Int, Int)] = {
+ if (stream.get(startIdx).getType != SqlBaseLexer.BEGIN) {
+ return None
+ }
+
+ var delimiter = fromDelimiter
+ while (delimiter <= delimiterPositions.length) {
+ val endIdx = if (delimiter < delimiterPositions.length) {
+ delimiterPositions(delimiter)
+ } else {
+ stream.size() - 1
+ }
+ val firstTok = stream.get(startIdx)
+ val lastTok = stream.get(endIdx)
+ val regionStart = toUtf16(firstTok.getStartIndex)
+ val regionEnd = if (lastTok.getType == Token.EOF) {
+ sqlText.length
+ } else {
+ toUtf16(lastTok.getStopIndex + 1)
+ }
+ val candidate = validationPreprocess(sqlText.substring(regionStart,
regionEnd))
+ val lexer = new SqlBaseLexer(
+ new UpperCaseCharStream(CharStreams.fromString(candidate)))
+ lexer.removeErrorListeners()
+ val tokens = new CommonTokenStream(lexer)
+ tokens.fill()
+ val parser = new SqlBaseParser(tokens)
+ configureSplitterParser(parser, conf, bailOnError = false)
+ parser.getInterpreter.setPredictionMode(PredictionMode.LL)
+ try {
+ val context = parser.singleCompoundStatement()
+ val end = context.END()
+ if (end != null && isOuterCompoundEnd(tokens, end.getSymbol)) {
+ return Some((delimiter, endIdx))
+ }
+ } catch {
+ case _: StackOverflowError => return None
+ }
+ delimiter += 1
+ }
+ None
+ }
+
+ /** Returns true only when a real recovered END closes the candidate's outer
BEGIN. */
+ private def isOuterCompoundEnd(tokens: CommonTokenStream, recoveredEnd:
Token): Boolean = {
+ if (recoveredEnd.getTokenIndex < 0) return false
+ val suffixEnd = trailingEndToken(tokens)
+ suffixEnd != null && closesOuterBegin(tokens, suffixEnd)
+ }
+
+ private def closesOuterBegin(tokens: CommonTokenStream, suffixEnd: Token):
Boolean = {
+ var depth = 0
+ var index = 0
+ val limit = suffixEnd.getTokenIndex
+ while (index <= limit) {
+ val token = tokens.get(index)
+ if (token.getChannel != Token.HIDDEN_CHANNEL) {
+ token.getType match {
+ case SqlBaseLexer.BEGIN =>
Review Comment:
**Blocking (P1):** This branch shows why the recovery proof should not be
extended with another token heuristic. `BEGIN` and `END` are non-reserved
identifiers, while nested compounds, labels, handlers, and control terminators
are grammar contexts; the review history has already found distinct failures in
flat fallback, recovered-`END` selection, suffix identity, and nested depth.
Please replace `findMalformedCompoundEnd` and its raw-token classifiers with a
grammar-owned, one-pass boundary parse for the parse_sql path, and keep generic
splitter behavior unchanged.
**Recommended change:** Replace the malformed-compound prefix-reparse and
token-heuristic layer with a dedicated, grammar-owned, one-pass batch-boundary
parser used only by parse_sql.
**Why this works:** Add a batch or recovery parser entry point whose error
strategy recovers malformed leaf statements at the semicolon owned by their
enclosing grammar context, but never synthesizes, deletes, or reclassifies
scripting structure. Only real grammar-matched BEGIN, END, control terminators,
and labels may open or close structural contexts. Derive PositionedSqlStatement
spans from the original token indices, then keep the existing per-segment
ParseSqlResult parsing.
**Scope:** SqlBaseParser grammar and generated parser integration; the
parse_sql-only positioned splitting path in SqlStatementSplitter or a dedicated
helper; SparkSqlParser wiring; focused SqlStatementSplitterSuite and
ParseSqlResultSuite coverage. The generic splitStatements path remains
unchanged.
**Compatibility:** Preserve current valid-batch results, UTF-16 spans,
SQL-whitespace trimming, empty/comment-only behavior, per-statement JSON
errors, and unclosed trailing partials. Do not change the generic splitter's
parser-extension and variable-substitution fallback.
**Risks:** An error strategy that resynchronizes at the wrong grammar depth
could consume a following top-level statement. The new batch entry point must
handle SET and RESET wildcard statements without making their terminators
ambiguous. Rejecting synthetic structural delimiters may classify structurally
unclosed scripts as partial rather than complete, which must match the existing
contract.
**Constraints:** Structural openers and closers must come from actual parser
terminals, not raw keyword spelling or synthetic recovery tokens. BEGIN and END
must remain valid non-reserved identifiers in both ANSI and default grammar
modes. Keep the existing one-time code-point-to-UTF-16 map and lexer-compatible
whitespace trimming. Do not consume a following top-level statement or alter
the default generic splitter route.
**Success:** One linear boundary parse per input identifies the same spans
for valid and ordinary-invalid batches; malformed balanced compounds remain one
result across nested BEGIN blocks, every END control form, labels, handlers,
and BEGIN or END identifier uses; structurally unclosed compounds remain
partial; and a following SELECT is always a separate correctly positioned
result.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/SqlStatementSplitter.scala:
##########
@@ -285,8 +370,155 @@ object SqlStatementSplitter {
val unclosed = lexer.has_unclosed_bracketed_comment
val partial =
- if (bufferHasContent || unclosed) buffer.toString.trim else ""
- SqlStatementSplitResult(completeStatements.toSeq, partial, unclosed &&
partial.nonEmpty)
+ if (bufferHasContent || unclosed) positionedStatement("") else None
+ PositionedSqlStatementSplitResult(
+ completeStatements.toSeq,
+ partial,
+ unclosed && partial.nonEmpty)
+ }
+
+ /**
+ * Returns the delimiter-array index and ending token index of a real outer
END for a malformed
Review Comment:
**Nit (P3):** `findMalformedCompoundEnd` returns `(delimiter, endIdx)`,
where `endIdx` is the semicolon token or EOF token for the candidate. It never
returns the recovered `END` token index. Could this document the second field
as the candidate-ending semicolon-or-EOF token index and describe the real
outer `END` only as the condition established before returning?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/SqlStatementSplitter.scala:
##########
@@ -285,8 +370,155 @@ object SqlStatementSplitter {
val unclosed = lexer.has_unclosed_bracketed_comment
val partial =
- if (bufferHasContent || unclosed) buffer.toString.trim else ""
- SqlStatementSplitResult(completeStatements.toSeq, partial, unclosed &&
partial.nonEmpty)
+ if (bufferHasContent || unclosed) positionedStatement("") else None
+ PositionedSqlStatementSplitResult(
+ completeStatements.toSeq,
+ partial,
+ unclosed && partial.nonEmpty)
+ }
+
+ /**
+ * Returns the delimiter-array index and ending token index of a real outer
END for a malformed
+ * compound statement. Error recovery may repair the body, but a missing END
is synthetic and
+ * has token index -1.
+ */
+ private def findMalformedCompoundEnd(
+ sqlText: String,
+ toUtf16: Array[Int],
+ stream: CommonTokenStream,
+ startIdx: Int,
+ delimiterPositions: Array[Int],
+ fromDelimiter: Int,
+ validationPreprocess: String => String,
+ conf: SqlApiConf): Option[(Int, Int)] = {
+ if (stream.get(startIdx).getType != SqlBaseLexer.BEGIN) {
+ return None
+ }
+
+ var delimiter = fromDelimiter
+ while (delimiter <= delimiterPositions.length) {
+ val endIdx = if (delimiter < delimiterPositions.length) {
+ delimiterPositions(delimiter)
+ } else {
+ stream.size() - 1
+ }
+ val firstTok = stream.get(startIdx)
+ val lastTok = stream.get(endIdx)
+ val regionStart = toUtf16(firstTok.getStartIndex)
+ val regionEnd = if (lastTok.getType == Token.EOF) {
+ sqlText.length
+ } else {
+ toUtf16(lastTok.getStopIndex + 1)
+ }
+ val candidate = validationPreprocess(sqlText.substring(regionStart,
regionEnd))
Review Comment:
**Non-blocking (P2):** This loop is also the performance consequence of the
same recovery design: every internal delimiter causes another substring,
preprocessing pass, lexer, token stream, and full-prefix parse. The
grammar-owned batch-boundary parser should consume the input once and remove
this candidate loop entirely, so a malformed compound with k internal
statements does O(k) boundary work per row rather than reparsing 1 + ... + k
prefixes.
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/expressions/ParseSql.scala:
##########
@@ -27,38 +27,46 @@ import org.apache.spark.sql.types.{AbstractDataType,
DataType, StringType}
import org.apache.spark.unsafe.types.UTF8String
/**
- * Parses a SQL statement string and returns a compact JSON description of the
- * unresolved statement (identifier/code, lineage references, select-list
names,
- * parameters), or a STANDARD-format error object when the statement does not
- * parse.
+ * Parses a SQL batch string and returns a compact JSON array describing its
+ * unresolved statements (source position, identifier/code, lineage references,
+ * select-list names, parameters). A statement that does not parse is
represented
Review Comment:
**Nit (P3):** The array element here is the statement-result wrapper
(`start`, `length`, `parse_success`, and `error`), not the STANDARD error
object itself; the latter is nested under `error`, as the ExpressionDescription
and generated output show. Could this say that an unparsable statement produces
a result object containing a nested STANDARD-format error object? Otherwise
readers may look for `$[0].errorClass` instead of `$[0].error.errorClass`.
--
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]
