spark-test commented on code in PR #59868:
URL: https://github.com/apache/spark/pull/59868#discussion_r4404694547
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/parser/ParseSqlResult.scala:
##########
@@ -123,8 +131,66 @@ object ParseSqlResult {
JObject(fields.toList)
}
+ /** 1-based Unicode code-point offset of `segment` in `batch`. */
+ private def charStart(batch: String, segment: PositionedSqlStatement): Int =
{
+ val utf16 = math.max(0, math.min(segment.start, batch.length))
+ batch.codePointCount(0, utf16) + 1
Review Comment:
**Non-blocking (P2):** `charStart` counts code points from offset 0 of the
batch for every statement (`batch.codePointCount(0, utf16)`), where the merge
base used `segment.start + 1`. It runs once per statement, from `successJson`
or `errorJson`, so for S statements in an N-unit batch the per-row work grows
from O(N) to roughly O(N + S*N/2).
The trigger is a batch whose decoded `String` is not compact Latin-1, that
is, any character above U+00FF anywhere in the row (a CJK comment, a smart
quote, or the emoji this PR targets). On Latin-1-coded strings `codePointCount`
returns without scanning, so ASCII and Latin-1 batches are unaffected, and rows
with a few statements pay only microseconds. For whole-script rows the cost is
material. By estimate (not measured), a 10^4-statement, 1 MB script adds about
1.5-5 s per row and a 10^5-statement, 10 MB one about 150-500 s, and such a row
becomes a straggler task that AQE cannot split.
The splitter already avoids this for the opposite conversion: `utf16Offsets`
exists to "Map once so each token-boundary lookup is O(1) rather than
rescanning the prefix". Statements are emitted in ascending start order and
every start falls on a code-point boundary. Carrying the previous UTF-16 offset
and a running code-point count from one statement to the next therefore yields
the same values in a single pass.
**Verification:**
- **Behavior:** Batches containing supplementary characters and BMP
whitespace still report the same 1-based code-point start and length for both
successful and failing statements, including a trailing partial statement.
- **Inspection:** No statement's code-point start is computed by counting
from the beginning of the batch; each count covers only the span since the
previous statement's start, so the whole batch is traversed at most once per
row.
##########
sql/core/src/test/scala/org/apache/spark/sql/catalyst/parser/ParseSqlResultSuite.scala:
##########
@@ -246,4 +253,32 @@ class ParseSqlResultSuite extends SparkFunSuite {
assert(j \ "parse_success" === JBool(false))
assert(j \ "error" \ "errorClass" === JString("PARSE_SYNTAX_ERROR"))
}
+
+ test("error queryContext indexes are Unicode code points") {
Review Comment:
**Nit (P3):** This test only checks `queryContext` for single statements at
batch offset 0 and never reads `start` or `length`. Every other
failing-statement case (in this suite, `ParseSqlSuite`, and `parse-sql.sql`,
including its `LABELS_MISMATCH` case) is ASCII-only, where UTF-16 units and
code points coincide. Two behaviors this PR introduces can therefore regress
without any test failing:
- **Failing-statement span.** Reverting `errorJson` to `segment.start + 1` /
`segment.length` passes every suite. A failing statement that both follows and
contains a supplementary character pins both values: the second statement of
`s"SELECT '$emoji'; SELECT '$emoji' FRUM t"` should report `start` 13 and
`length` 17 (UTF-16 gives 14 and 18). An ASCII-only failing statement such as
`SELEC 2` has the same length in both units, so it cannot catch a UTF-16
`length`.
- **Scripting-error contexts.** `SqlScriptingException` keeps the default
empty `getQueryContext`, so its context is added by `queryContextField`, whose
`fragment` is a UTF-16 slice. It comes out right only because
`rewriteErrorCharIndexes` runs on the merged error object. For `BEGIN SELECT
'$emoji'; lbl_begin: BEGIN SELECT 1; END lbl_end; END`, `LABELS_MISMATCH`
should report indexes 19..28 with fragment `'lbl_begin:'`. If the rewrite ran
before the merge, the fragment would be `' lbl_begin'` and nothing would fail.
**Verification:**
- **Regression:** With a failing statement that follows a supplementary
character and contains one itself, the new assertions pass on the current
code-point start and length, and each would fail on its own if the error object
reported that value in UTF-16 units.
- **Regression:** With a scripting label mismatch after a supplementary
character, the new assertion passes on the code-point fragment and indexes and
would fail if scripting contexts kept the UTF-16-sliced fragment.
--
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]
