[
https://issues.apache.org/jira/browse/SPARK-60672?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
ASF GitHub Bot updated SPARK-60672:
-----------------------------------
Labels: pull-request-available (was: )
> 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
> Priority: Major
> Labels: pull-request-available
>
> 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]
