cloud-fan commented on code in PR #59868:
URL: https://github.com/apache/spark/pull/59868#discussion_r4420355155
##########
sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionNode.scala:
##########
@@ -185,8 +185,8 @@ class SingleStatementExec(
* SQL query text.
*/
def getText: String = {
- assert(origin.sqlText.isDefined && origin.startIndex.isDefined &&
origin.stopIndex.isDefined)
- origin.sqlText.get.substring(origin.startIndex.get, origin.stopIndex.get +
1)
+ assert(origin.sqlTextSlice.isDefined)
Review Comment:
**Nit (P3):** This method and `ResolveExecuteImmediate.extractOriginalText`
now slice statement text through `sqlTextSlice`, but no test drives either call
site with a supplementary character before the sliced text. The new tests cover
the `sqlTextSlice` helper and the catalyst `SingleStatement.getText`, which has
no production caller. If either call site went back to `substring(startIndex,
stopIndex + 1)`, every suite would still pass. Two user-facing errors would
then report shifted text after a supplementary character:
INVALID_BOOLEAN_STATEMENT (and the more-than-one-row internal error) from this
method, and the variable name in NULL_QUERY_STRING_EXECUTE_IMMEDIATE. For
example, in `BEGIN SELECT '<U+1F600>'; IF 1 THEN SELECT 1; END IF; END`, the
integer condition takes the non-boolean branch, and a reverted slice would name
the text one UTF-16 unit to the left of the condition.
**Verification:**
- **Regression:** A syntax error in a parameterized query whose SQL text or
parameter value contains a supplementary character, and whose text continues
after its last parameter marker, keeps a whole-text query context that covers
the full original SQL with a code-point stopIndex, and the check fails if the
parameterized-query origin goes back to a UTF-16 length.
- **Regression:** In a SQL script with a supplementary character before a
non-boolean IF condition, the INVALID_BOOLEAN_STATEMENT error names exactly the
condition text, and the check fails if the statement text is sliced in UTF-16
units.
- **Regression:** For EXECUTE IMMEDIATE over a null string variable with a
supplementary character earlier in the statement, the
NULL_QUERY_STRING_EXECUTE_IMMEDIATE error names exactly the variable
expression, and the check fails if the original text is sliced in UTF-16 units.
- **Compatibility:** The same three errors for SQL without supplementary
characters keep their existing query contexts and message parameters.
- **Inspection:** The SQL-scripting CASE-condition origin computes its
stopIndex from the condition text's code-point count, consistent with the
renderer's code-point validity check.
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/parser/ParseSqlResult.scala:
##########
@@ -123,9 +134,85 @@ object ParseSqlResult {
JObject(fields.toList)
}
+ /**
+ * 1-based Unicode code-point starts for `statements` in `batch`. Statements
+ * are in ascending UTF-16 start order, so one left-to-right pass is enough:
+ * each count covers only the gap since the previous start.
+ */
+ private def charStarts(
+ batch: String,
+ statements: Seq[PositionedSqlStatement]): Seq[Int] = {
+ var utf16 = 0
+ var codePoints = 0
+ statements.map { segment =>
+ val startUtf16 = math.max(0, math.min(segment.start, batch.length))
+ if (startUtf16 >= utf16) {
+ codePoints += batch.codePointCount(utf16, startUtf16)
+ } else {
+ codePoints = batch.codePointCount(0, startUtf16)
+ }
+ utf16 = startUtf16
+ codePoints + 1
+ }
+ }
+
+ /** Unicode code-point length of the trimmed statement text. */
+ private def charLength(segment: PositionedSqlStatement): Int = {
+ val text = segment.statement
+ text.codePointCount(0, text.length)
+ }
+
+ /**
+ * Keep statement-level `queryContext` indexes inside the statement.
+ * In-range contexts keep the shared renderer's fragment. A context that
+ * falls outside the statement is widened to cover all of it. Contexts of
+ * another object (a view body) are left as is.
+ */
+ private def rewriteErrorCharIndexes(errorObj: JObject, statement: String):
JObject = {
+ JObject(errorObj.obj.map {
+ case ("queryContext", JArray(contexts)) =>
+ "queryContext" -> JArray(contexts.map {
+ case ctx: JObject => rewriteQueryContextIndexes(ctx, statement)
+ case other => other
+ }.toList)
+ case other => other
+ })
+ }
+
+ private def rewriteQueryContextIndexes(ctx: JObject, statement: String):
JObject = {
+ val objectType = ctx.obj.collectFirst { case ("objectType", JString(t)) =>
t }
+ val start = ctx.obj.collectFirst { case ("startIndex", JInt(n)) => n.toInt
}
+ val stop = ctx.obj.collectFirst { case ("stopIndex", JInt(n)) => n.toInt }
+ (objectType, start, stop) match {
+ case (Some(""), Some(from), Some(to)) =>
+ val length = statement.codePointCount(0, statement.length)
+ if (from >= 1 && to >= from && to <= length) {
+ ctx
+ } else {
+ JObject(ctx.obj.map {
+ case ("startIndex", _) => "startIndex" -> JInt(1)
+ case ("stopIndex", _) => "stopIndex" -> JInt(length)
+ case ("fragment", _) =>
+ "fragment" -> JString(codePointSlice(statement, 0, length))
Review Comment:
**Nit (P3):** This is the only call of `codePointSlice`, and `length` is the
statement's full code-point count from line 188, so the call always returns
`statement` unchanged. A jshell check over empty, ASCII, emoji, CJK, and
unpaired-surrogate strings confirms it. The helper still runs two
`offsetByCodePoints` walks from offset 0, which are linear even on Latin-1
text. It also leaves a second clamped code-point slicer, with an exclusive
bound, next to the inclusive `SQLQueryContext.sliceByCodePoints`, which invites
bound mix-ups later. Using `JString(statement)` here and deleting
`codePointSlice` produces the same JSON.
**Verification:**
- **Inspection:** The widening branch emits the statement string directly,
and no code-point slicing helper remains in parse_sql's result builder.
- **Behavior:** parse_sql error JSON for in-range statement-level contexts,
including those after a supplementary character, is unchanged.
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/parser/ParseSqlResult.scala:
##########
@@ -123,9 +134,83 @@ object ParseSqlResult {
JObject(fields.toList)
}
+ /**
+ * 1-based Unicode code-point starts for `statements` in `batch`. Statements
+ * are in ascending UTF-16 start order, so one left-to-right pass is enough:
+ * each count covers only the gap since the previous start.
+ */
+ private def charStarts(
+ batch: String,
+ statements: Seq[PositionedSqlStatement]): Seq[Int] = {
+ var utf16 = 0
+ var codePoints = 0
+ statements.map { segment =>
+ val startUtf16 = math.max(0, math.min(segment.start, batch.length))
+ if (startUtf16 >= utf16) {
+ codePoints += batch.codePointCount(utf16, startUtf16)
+ } else {
+ codePoints = batch.codePointCount(0, startUtf16)
+ }
+ utf16 = startUtf16
+ codePoints + 1
+ }
+ }
+
+ /** Unicode code-point length of the trimmed statement text. */
+ private def charLength(segment: PositionedSqlStatement): Int = {
+ val text = segment.statement
+ text.codePointCount(0, text.length)
+ }
+
+ /**
+ * Recompute the fragment of each statement `queryContext` by code points.
+ * The indexes are parser (code-point) offsets, but [[SQLQueryContext]]
+ * slices its fragment by UTF-16 units, which is off after a supplementary
+ * character. A context that falls outside the statement covers all of it.
+ * Contexts of another object (a view body) are left as is.
+ */
+ private def rewriteErrorCharIndexes(errorObj: JObject, statement: String):
JObject = {
+ JObject(errorObj.obj.map {
+ case ("queryContext", JArray(contexts)) =>
+ "queryContext" -> JArray(contexts.map {
+ case ctx: JObject => rewriteQueryContextIndexes(ctx, statement)
+ case other => other
+ }.toList)
+ case other => other
+ })
+ }
+
+ private def rewriteQueryContextIndexes(ctx: JObject, statement: String):
JObject = {
+ val objectType = ctx.obj.collectFirst { case ("objectType", JString(t)) =>
t }
+ val start = ctx.obj.collectFirst { case ("startIndex", JInt(n)) => n.toInt
}
+ val stop = ctx.obj.collectFirst { case ("stopIndex", JInt(n)) => n.toInt }
+ (objectType, start, stop) match {
+ case (Some(""), Some(from), Some(to)) =>
+ val length = statement.codePointCount(0, statement.length)
+ val (first, last) =
+ if (from >= 1 && to >= from && to <= length) (from, to) else (1,
length)
+ JObject(ctx.obj.map {
+ case ("startIndex", _) => "startIndex" -> JInt(first)
+ case ("stopIndex", _) => "stopIndex" -> JInt(last)
+ case ("fragment", _) => "fragment" ->
JString(codePointSlice(statement, first - 1, last))
Review Comment:
Thanks, in-range statement contexts now keep the shared renderer's fragment,
so the duplicate re-slicing is gone. The test gap is only partly closed. The
new EXECUTE IMMEDIATE syntax-error case pins that origin's code-point
stopIndex, but the parameterized-query whole-text origin still has no test that
fails if it goes back to a UTF-16 length, because the new mapping test builds
its origin by hand.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4407271872","thread_id":"inline:4407271872","verdict_sha256":"f35e097fd29f69564279f7c0e4f050b10aecc4cc8e7ee90355258520d6180b2f"}
-->
--
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]
