SEPURI-SAI-KRISHNA commented on PR #57931:
URL: https://github.com/apache/spark/pull/57931#issuecomment-5587575326
@stevomitric good catch, you are right, and it is a real bug. Fixed.
With `FOR 0` the tail of the result starts at `pos + length`, which for `pos
= -2147483648` is `Int.MinValue` itself. `UTF8String.substringSQL` then
computes `start = numChars + pos = 9 + Int.MinValue = -2147483639` and `end =
start + Int.MaxValue = 8`. `substring` only treats the end offset as "the rest
of the input" when it is exactly `Int.MaxValue`, so it returned characters `[0,
8)` and dropped the final `L`.
The binary overload took a different route to a different answer:
`bytes.length - start` overflows the `int` range, the `< len` test therefore
passes, and `end` becomes `bytes.length`, so it returned the whole input. Hence
the mismatch you predicted, `_Spark SQ` against `_Spark SQL`.
It was also inconsistent with the case this PR already tests: `FOR 1` gives
`_Spark SQL`, because the tail start is `Int.MinValue + 1` and the arithmetic
happens to land on `end = 9`. The same position gave a different trailing
character depending only on the `FOR` value.
The fix clamps the tail position to `-length(input)`. Every position at or
before the start of the input denotes the same tail, the whole input, so this
removes the discontinuity without touching the negative-index semantics for
positions inside the input. I added the zero-length case for both overloads,
and `Int.MinValue + 1` with `FOR 0` as well.
Worth flagging that #57922 would have hidden this rather than fixed it: it
rewrites `ByteArray.subStringSQL` to clamp the end offset with `Math.min`,
which makes the binary overload return `_Spark SQ` too. The two overloads would
then agree, on the wrong answer.
Two related things I checked while fixing this, in case they are worth
separate tickets.
`substring` has the same discontinuity on its own, without going through
`overlay`: `substring('Spark SQL', -2147483648, 2147483647)` returns `Spark
SQ`, while `substring('Spark SQL', -2147483647, 2147483647)` returns `Spark
SQL`. One position apart, one character different, for the same reason. I left
it alone because it is outside this PR, but happy to file it if you agree it is
wrong.
`overlay('Spark SQL' PLACING '_' FROM -2147483648 FOR 2147483647)` returns
`_L`. Here `pos + length` is exactly `-1` in `long` arithmetic, with no
overflow, and a negative tail position means "from the end" under the
convention this expression already follows, the same one that makes the
existing `Overlay(..., -10)` case return `__park SQL` rather than `__Spark
SQL`. So this PR does not change that behaviour, it only stops the input being
duplicated around the replacement. Let me know if you would rather that
convention changed too, but it looked like a separate discussion.
I also added the multi-byte and empty-input cases as regression coverage,
since the tail is clamped in characters rather than bytes.
--
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]
