cloud-fan commented on code in PR #55466:
URL: https://github.com/apache/spark/pull/55466#discussion_r3312927448
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLCLIDriver.scala:
##########
@@ -613,110 +613,65 @@ private[hive] class SparkSQLCLIDriver extends CliDriver
with Logging {
}
}
- // Adapted splitSemiColon from Hive 2.3's CliDriver.splitSemiColon.
- // Note: [SPARK-31595] if there is a `'` in a double quoted string, or a `"`
in a single quoted
- // string, the origin implementation from Hive will not drop the trailing
semicolon as expected,
- // hence we refined this function a little bit.
- // Note: [SPARK-33100] Ignore a semicolon inside a bracketed comment in
spark-sql.
+ // Structural scanner for splitting SQL by semicolons.
+ // Handles: quoted strings with escapes, line comments (--), nested block
comments (/* */),
+ // and semicolons inside strings/comments are not treated as delimiters.
+ // Note: [SPARK-31595], [SPARK-33100], [SPARK-54876]
private[hive] def splitSemiColon(line: String): JList[String] = {
- var insideSingleQuote = false
- var insideDoubleQuote = false
- var insideSimpleComment = false
- var bracketedCommentLevel = 0
- var escape = false
- var beginIndex = 0
- var leavingBracketedComment = false
- var isStatement = false
val ret = new JArrayList[String]
-
- def insideBracketedComment: Boolean = bracketedCommentLevel > 0
- def insideComment: Boolean = insideSimpleComment || insideBracketedComment
- def statementInProgress(index: Int): Boolean = isStatement ||
(!insideComment &&
- index > beginIndex && !s"${line.charAt(index)}".trim.isEmpty)
-
- for (index <- 0 until line.length) {
- // Checks if we need to decrement a bracketed comment level; the last
character '/' of
- // bracketed comments is still inside the comment, so
`insideBracketedComment` must keep true
- // in the previous loop and we decrement the level here if needed.
- if (leavingBracketedComment) {
- bracketedCommentLevel -= 1
- leavingBracketedComment = false
+ val n = line.length
+ var i = 0
+ var chunkStart = 0
+ var chunkHasSql = false
+ var chunkHasUnclosed = false
+
+ def consumeString(start: Int, quote: Char): Int = {
+ var p = start + 1
+ while (p < n) {
+ val c = line.charAt(p)
+ if (c == '\\' && p + 1 < n) p += 2
+ else if (c == quote) return p + 1
+ else p += 1
}
+ chunkHasUnclosed = true; n
+ }
- if (line.charAt(index) == '\'' && !insideComment) {
- // take a look to see if it is escaped
- // See the comment above about SPARK-31595
- if (!escape && !insideDoubleQuote) {
- // flip the boolean variable
- insideSingleQuote = !insideSingleQuote
- }
- } else if (line.charAt(index) == '\"' && !insideComment) {
- // take a look to see if it is escaped
- // See the comment above about SPARK-31595
- if (!escape && !insideSingleQuote) {
- // flip the boolean variable
- insideDoubleQuote = !insideDoubleQuote
- }
- } else if (line.charAt(index) == '-') {
- val hasNext = index + 1 < line.length
- if (insideDoubleQuote || insideSingleQuote || insideComment) {
- // Ignores '-' in any case of quotes or comment.
- // Avoids to start a comment(--) within a quoted segment or already
in a comment.
- // Sample query: select "quoted value --"
- // ^^ avoids starting a comment
if it's inside quotes.
- } else if (hasNext && line.charAt(index + 1) == '-') {
- // ignore quotes and ; in simple comment
- insideSimpleComment = true
- }
- } else if (line.charAt(index) == ';') {
- if (insideSingleQuote || insideDoubleQuote || insideComment) {
- // do not split
- } else {
- if (isStatement) {
- // split, do not include ; itself
- ret.add(line.substring(beginIndex, index))
- }
- beginIndex = index + 1
- isStatement = false
- }
- } else if (line.charAt(index) == '\n') {
- // with a new line the inline simple comment should end.
- if (!escape) {
- insideSimpleComment = false
- }
- } else if (line.charAt(index) == '/' && !insideSimpleComment) {
- val hasNext = index + 1 < line.length
- if (insideSingleQuote || insideDoubleQuote) {
- // Ignores '/' in any case of quotes
- } else if (insideBracketedComment && line.charAt(index - 1) == '*' ) {
- // Decrements `bracketedCommentLevel` at the beginning of the next
loop
- leavingBracketedComment = true
- } else if (hasNext && line.charAt(index + 1) == '*') {
- bracketedCommentLevel += 1
- }
- }
- // set the escape
- if (escape) {
- escape = false
- } else if (line.charAt(index) == '\\') {
- escape = true
+ def consumeLineComment(start: Int): Int = {
+ var p = start + 2
+ while (p < n && line.charAt(p) != '\n') p += 1
+ p
+ }
+
+ def consumeBlockComment(start: Int): Int = {
+ var p = start + 2
+ var level = 1
+ while (p + 1 < n && level > 0) {
+ val c0 = line.charAt(p); val c1 = line.charAt(p + 1)
+ if (c0 == '/' && c1 == '*') { level += 1; p += 2 }
+ else if (c0 == '*' && c1 == '/') { level -= 1; p += 2 }
+ else p += 1
}
+ if (level > 0) { chunkHasUnclosed = true; n } else p
+ }
- isStatement = statementInProgress(index)
- }
- // Check the last char is end of nested bracketed comment.
- val endOfBracketedComment = leavingBracketedComment &&
bracketedCommentLevel == 1
- // Spark SQL support simple comment and nested bracketed comment in query
body.
- // But if Spark SQL receives a comment alone, it will throw parser
exception.
- // In Spark SQL CLI, if there is a completed comment in the end of whole
query,
- // since Spark SQL CLL use `;` to split the query, CLI will pass the
comment
- // to the backend engine and throw exception. CLI should ignore this
comment,
- // If there is an uncompleted statement or an uncompleted bracketed
comment in the end,
- // CLI should also pass this part to the backend engine, which may throw
an exception
- // with clear error message.
- if (!endOfBracketedComment && (isStatement || insideBracketedComment)) {
- ret.add(line.substring(beginIndex))
+ while (i < n) {
+ val c = line.charAt(i)
+ def peek(ch: Char): Boolean = i + 1 < n && line.charAt(i + 1) == ch
+ if (c == '\'' || c == '"' || c == '`') {
+ chunkHasSql = true; i = consumeString(i, c)
+ } else if (c == '-' && peek('-')) {
+ i = consumeLineComment(i)
+ } else if (c == '/' && peek('*')) {
+ i = consumeBlockComment(i)
+ } else if (c == ';') {
+ if (chunkHasSql) ret.add(line.substring(chunkStart, i))
+ chunkStart = i + 1; chunkHasSql = false; chunkHasUnclosed = false; i
+= 1
+ } else {
+ if (!Character.isWhitespace(c)) chunkHasSql = true
+ i += 1
+ }
}
+ if (chunkHasSql || chunkHasUnclosed) ret.add(line.substring(chunkStart))
ret
}
Review Comment:
The two `splitSemiColon` implementations are now byte-identical (modulo
return type and the `enableSqlScripting` flag). My prior review flagged
"Consolidation. Both `SparkSQLCLIDriver.splitSemiColon` and
`StringUtils.splitSemiColonWithIndex` become call sites for one implementation"
as a structural win -- now that the bodies have converged, the merge is one
mechanical replacement:
```suggestion
// Splits SQL into individual statements by top-level semicolons. See
// [[StringUtils.splitSemiColonWithIndex]] for the implementation.
// Note: [SPARK-31595], [SPARK-33100], [SPARK-54876]
private[hive] def splitSemiColon(line: String): JList[String] =
StringUtils.splitSemiColonWithIndex(line, enableSqlScripting =
false).asJava
```
Also add `import org.apache.spark.sql.catalyst.util.StringUtils` near the
other `catalyst.util` import. All SPARK-37906 CliSuite cases continue to pass
(the two implementations are functionally identical with
`enableSqlScripting=false`). As a free side-effect, this also fixes the
backtick gap noted in the other comment, since StringUtils has always treated
`` ` `` as a quote.
##########
sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/CliSuite.scala:
##########
@@ -674,7 +674,16 @@ class CliSuite extends SparkFunSuite {
"SELECT /* comment */ 1;" -> Seq("SELECT /* comment */ 1"),
"-- comment " -> Seq(),
"-- comment \nSELECT 1" -> Seq("-- comment \nSELECT 1"),
- "/* comment */ " -> Seq()
+ "/* comment */ " -> Seq(),
+ // SPARK-54876: statement after semicolon ending with block comment
should not be dropped
+ "SELECT 1; SELECT 2 /* comment */" -> Seq("SELECT 1", " SELECT 2 /*
comment */"),
+ // SPARK-54876: line comment followed by block comment should produce
empty result
+ "-- foo\n/* bar */" -> Seq(),
+ "SELECT 1; -- foo\n /* bar */" -> Seq("SELECT 1"),
+ // SPARK-54876: nested block comments
+ "SELECT 1; /* outer /* inner */ */" -> Seq("SELECT 1"),
+ // SPARK-54876: preceding closed block comment + line comment (no SQL
statement)
+ "/* a */ -- foo\n/* b */" -> Seq()
Review Comment:
Pre-PR, `SparkSQLCLIDriver.splitSemiColon` did not treat `` ` `` as a quote
(only `'` and `"`). The new scanner does (`c == '\'' || c == '"' || c == '`'`),
so `;` inside a backtick-quoted identifier no longer splits. This is a fix --
matches ANSI identifier quoting and what `StringUtils.splitSemiColonWithIndex`
always did -- but it's not mentioned in the PR description and has no test
here. Add a case to lock it in:
```suggestion
// SPARK-54876: preceding closed block comment + line comment (no SQL
statement)
"/* a */ -- foo\n/* b */" -> Seq(),
// SPARK-54876: semicolons inside backtick-quoted identifiers are not
split points
"SELECT * FROM `t;a`; SELECT 1" -> Seq("SELECT * FROM `t;a`", " SELECT
1")
```
(After applying the consolidation suggestion on the main file, this test
verifies that the delegation preserves the backtick behavior end-to-end.)
--
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]
