Serge Rielau created SPARK-60672:
------------------------------------
Summary: Fix error offsets to use proepr codepoints
Key: SPARK-60672
URL: https://issues.apache.org/jira/browse/SPARK-60672
Project: Spark
Issue Type: Bug
Components: SQL
Affects Versions: 5.0.0
Reporter: Serge Rielau
Things get strictly better wherever supplementary characters (emoji and other
characters above U+FFFF) are involved, and nothing changes for text without
them. Named-parameter substitution and procedure parameters can't regress in
their results, because the proposed changes only touch error rendering. The
real risk is narrower: it lies in doing the change incompletely.
What is broken today. I ran a throwaway probe on the current branch, which
renders errors the same way as master; I didn't commit it. Every error after a
supplementary character is already wrong on the regular SQL path:
||Case||Fragment reported today||Correct||
|{{SELECT '😀' AS a, DATE 'bad'}}|{{DATE 'bad}}|{{DATE 'bad'}}|
|Same with ASCII {{'x'}}|{{DATE 'bad'}}|ok|
|{{{}SELECT :p AS a, DATE 'bad'{}}}, {{p = '😀'}}|{{DATE 'bad}} (start 15)|start
16|
|Same with {{p = 'x'}}|{{DATE 'bad'}} (start 16)|ok|
|Positional {{?}} with {{'😀'}}|{{DATE 'bad}}|wrong|
|Unresolved column after a named {{'😀'}} value|{{no_such_co}}|wrong|
|{{EXECUTE IMMEDIATE}} with an emoji|{{DATE 'bad}}|wrong|
|Unresolved column after an emoji literal|{{no_such_co}}|wrong|
The {{PRETTY}} caret is one column left in each wrong case. The parameter rows
matter most: the SQL text is pure ASCII, yet the position is still wrong. That
is because {{PositionMapper}} takes substitution bounds from ANTLR (code
points) but measures the replacement with {{String.length}} (UTF-16 units).
Why substitution and procedure parameters are safe.
* The text substitution itself, {{{}applySubstitutions{}}}, already converts
with {{{}offsetByCodePoints{}}}, and the existing emoji tests in
{{ParameterSubstitutionSuite}} cover it. The proposal doesn't touch it.
* {{PositionMapper}} is only used in {{{}Origin.context{}}}, which builds
error contexts. It plays no part in binding, so fixing it can only change error
text.
* Positional markers get their identity from ANTLR
({{{}PosParameter(getStartIndex){}}}). Those indexes stay the same.
* Procedure, SQL-function and script parameter resolution never reads
{{Origin}} indexes.
Where it is strictly no-change. If the text has no surrogate pairs, every
code-point index equals its UTF-16 index. So the fragment, caret and validity
check are the same for all ASCII, Latin-1 and other BMP text, which is nearly
every workload. Spark has no golden or test with a supplementary character in
an error context, so no expected outputs change.
Real risks:
# The renderer and its producers must change together.
** Three places set {{stopIndex}} from a UTF-16 length: parameterized queries
({{{}SparkSqlParser{}}}), {{{}EXECUTE IMMEDIATE{}}}, and scripting simple-CASE
conditions. {{parse_sql}} is already in code points.
** If the renderer switches but one producer doesn't, that producer's
whole-text context fails the stricter validity check and is dropped for text
with emoji. You would get no context, not a wrong one. Tests for each producer
catch this.
# Four places the reviewer didn't mention. These slice SQL text with
{{{}substring(startIndex, stopIndex + 1){}}}, outside the renderer:
** {{SingleStatement.getText}}
** {{SingleStatementExec.getText}}
** {{ResolveExecuteImmediate.extractOriginalText}}
** the pipelines file registration, which also reads {{origin.startIndex}}
The first three only feed error messages, and they are already wrong after an
emoji because parser indexes are code points. If the CASE-condition producer
switches to code points, {{SingleStatementExec.getText}} would cut those
conditions short, but in practice that path can't be reached ({{{}x = v{}}} is
always a single-row BOOLEAN). They should move to code-point slicing in the
same change; that also fixes them.
# External clients that slice text themselves. Parser-origin indexes don't
change. For the three whole-text contexts, {{stopIndex}} drops by the number of
supplementary characters. A Java or JavaScript consumer that slices by UTF-16
would then see that context short by that many characters, but that consumer is
already wrong for every parser-origin context. Python slices by code points, so
it gets more correct. I can't verify how Databricks editor highlighting reads
these indexes; it's worth checking separately.
# Edge and cost. A context whose code-point stop lies past the text's
code-point length is now dropped instead of rendered as garbage. Nothing else
costs more: the extra code-point walk only runs when an error context is
rendered.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
