cloud-fan commented on code in PR #59868:
URL: https://github.com/apache/spark/pull/59868#discussion_r4414373670
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/SQLQueryContextSuite.scala:
##########
@@ -45,5 +45,31 @@ class SQLQueryContextSuite extends SparkFunSuite with
SQLHelper {
).foreach { case ((sqlText, start, stop), expectedFragment) =>
assert(getFragment(sqlText, start, stop) === expectedFragment)
}
+
+ val emoji = new String(Character.toChars(0x1F600))
+ val sql = s"SELECT '$emoji' AS a, DATE 'bad'"
+ val start = sql.codePointCount(0, sql.indexOf("DATE"))
+ val stop = sql.codePointCount(0, sql.length) - 1
+ assert(getFragment(sql, start, stop) === "DATE 'bad'")
+ // A UTF-16 slice of the same indexes would include the comma and drop the
last quote.
+ assert(sql.substring(start, stop + 1) !== "DATE 'bad'")
+ }
+
+ test("summary caret follows code-point indexes after a supplementary
character") {
Review Comment:
**Nit (P3):** This test passes unchanged against the merge-target `summary`.
For a context on the text's first line, the old code starts the carets one
UTF-16 unit early but also emits two spaces for the emoji, so the two errors
cancel: both versions render 17 spaces and then 10 carets for this input, and
the caret assertion in `QueryContextSuite` matches both as well. The new
supplementary-text branch of `summary`, including its start/stop conversion and
per-code-point loop, could therefore regress without any test failing. The
outputs differ only for a context on a later line than a supplementary
character, such as `DATE 'bad'` on the line after `SELECT '<emoji>' AS a,` (the
merge target draws 9 carets, this PR draws 10), or for a start-truncated
context with a supplementary character before it.
**Verification:**
- **Regression:** For a context on a later line than a supplementary
character, such as DATE 'bad' on the line after SELECT '<U+1F600>' AS a, the
summary caret underlines every code point of the context; the merge-target
summary drops the last one.
- **Regression:** For a context that follows other text on a later line than
a supplementary character, such as DATE 'bad' after ' b, ' on the line after
SELECT '<U+1F600>' AS a,, the caret starts under the context's first code
point; the merge-target summary starts it one column to the left.
- **Behavior:** A context whose fragment contains or ends with a
supplementary character renders exactly one caret per code point of the
fragment and none beyond it.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/SQLQueryContextSuite.scala:
##########
@@ -45,5 +45,31 @@ class SQLQueryContextSuite extends SparkFunSuite with
SQLHelper {
).foreach { case ((sqlText, start, stop), expectedFragment) =>
assert(getFragment(sqlText, start, stop) === expectedFragment)
}
+
+ val emoji = new String(Character.toChars(0x1F600))
+ val sql = s"SELECT '$emoji' AS a, DATE 'bad'"
+ val start = sql.codePointCount(0, sql.indexOf("DATE"))
+ val stop = sql.codePointCount(0, sql.length) - 1
+ assert(getFragment(sql, start, stop) === "DATE 'bad'")
+ // A UTF-16 slice of the same indexes would include the comma and drop the
last quote.
Review Comment:
**Nit (P3):** Here `sql.substring(start, stop + 1)` is `" DATE 'bad"`.
UTF-16 index 16 is the comma and index 17 is the space before `DATE`, so the
UTF-16 slice picks up that space, not the comma, and drops the closing quote.
The assertion still passes, but the comment misleads readers about how the
shift shows up.
```suggestion
// A UTF-16 slice of the same indexes would include a leading space and
drop the last quote.
```
**Verification:**
- **Inspection:** The comment describes the UTF-16 slice as picking up the
space before DATE and dropping the closing quote, which matches the asserted
expression.
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/trees/QueryContexts.scala:
##########
@@ -19,6 +19,25 @@ package org.apache.spark.sql.catalyst.trees
import org.apache.spark.{QueryContext, QueryContextType,
SparkUnsupportedOperationException}
+object SQLQueryContext {
+
+ /**
+ * Inclusive Unicode code-point slice `[from, to]` of `text`. Indexes are
clamped to the text.
+ * Parser origins store ANTLR code-point offsets, so this is the unit every
SQL-text recovery
Review Comment:
**Nit (P3):** This scaladoc is now the only place that says query-context
indexes are code points. The public `QueryContext` interface, which
`SparkThrowable.getQueryContext` returns to users, still describes
`startIndex()` and `stopIndex()` only as "The index starts from 0", in line
comments that the published API docs omit, and the PySpark docstrings say the
same. After this PR, whole-text contexts and positions mapped past a parameter
value also count code points. For example, the syntax error in
`spark.sql("SELECT '<emoji>', :p FRUM t", Map("p" -> 1))` now reports
`stopIndex()` 20 instead of 21, so a JVM caller that slices its own SQL
`String` with `substring(startIndex(), stopIndex() + 1)` loses the last
character. Javadoc on `startIndex()` and `stopIndex()` stating that they are
0-based code-point offsets with an inclusive stop, plus the same note in the
PySpark docstrings, would close the gap; the Connect proto field comments can
follow separately.
**Verification:**
- **Inspection:** The QueryContext startIndex/stopIndex documentation in
Java and in PySpark names Unicode code points as the unit and says stopIndex is
inclusive.
##########
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, this fixes the main issue: SQLQueryContext, the whole-text origins,
and PositionMapper now all use code points, and the new QueryContextSuite cases
cover the regular SQL path and a parameter value that contains an emoji. One
part is still open. parse_sql still replaces the fragment of every in-range
statement-level context with its own code-point slice, so the per-context
re-slicing is still there even though the renderer now produces the same
fragment. Only the out-of-range widening needs to rewrite the fragment. This
review also raises that the new tests do not exercise the whole-text stopIndex
changes.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4407271872","thread_id":"inline:4407271872","verdict_sha256":"0f38303c309a31165f841b91c941a84afa47055a3e530b76a007588847464a98"}
-->
--
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]
