You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@spark.apache.org by ur...@apache.org on 2026/09/09 09:05:08 UTC
(spark) branch branch-4.0 updated: [SPARK-58708][SQL] Fix out-of-range length handling in substring/lpad/rpad
This is an automated email from the ASF dual-hosted git repository.
uros-b pushed a commit to branch branch-4.0
in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/branch-4.0 by this push:
new 22e398ec21fa [SPARK-58708][SQL] Fix out-of-range length handling in substring/lpad/rpad
22e398ec21fa is described below
commit 22e398ec21faab061bf435a6a7b227068c070bec
Author: Sepuri Sai Krishna <sa...@gmail.com>
AuthorDate: Wed Sep 9 09:04:09 2026 +0000
[SPARK-58708][SQL] Fix out-of-range length handling in substring/lpad/rpad
### What changes were proposed in this pull request?
This PR fixes three integer-overflow / missing-guard bugs in the argument handling of
`substring()`, `lpad()` and `rpad()`, all of which let an out-of-range `len` argument reach an
allocation it should never have reached.
`ByteArray.subStringSQL` (the BINARY implementation of `substring()`) computed its end offset in
`int` arithmetic:
```java
if ((bytes.length - start) < len) { // overflows when len is large negative
end = bytes.length;
} else {
end = start + len; // overflows when start + len > Integer.MAX_VALUE
}
start = Math.max(start, 0); // start clamped only AFTER it was used above
```
When `start` is still negative, `bytes.length - start` overflows, the `end = bytes.length` clamp is
skipped, and `end` keeps an out-of-range value. `Arrays.copyOfRange` accepts an end past the array
length and zero-pads the remainder, so the result is silently a large array of zero bytes. This is
now computed in `long` and clamped to `bytes.length`, with `start` clamped only afterwards.
`ByteArray.lpad` / `ByteArray.rpad` guarded only the exact value `0` before `new byte[len]`, so a
negative `len` threw `NegativeArraySizeException`. The guard is now `len <= 0`, which also makes
`padWithEmptyPattern` unreachable with a non-positive length.
`UTF8String.lpad` / `UTF8String.rpad` start with `int spaces = len - numChars()`, which wraps to a
large positive value when `len == Integer.MIN_VALUE`. That skips the `spaces <= 0` branch and takes
the padding branch, failing in `Math.toIntExact`. Both now return the empty string up front for
`len <= 0`, before the wrapping subtraction is evaluated.
With all three in place, a non-positive length yields the empty value for STRING and BINARY alike
across the whole `int` range, and no argument combination can produce a result longer than the
input.
### Why are the changes needed?
All three are reachable from plain SQL with default configuration, on both the interpreted and
codegen paths.
```sql
-- 1. wrong results, and an allocation unrelated to the size of the input
SELECT length(substring(CAST('Spark SQL' AS BINARY), -1207959552, -1207959552));
-- before: 1879048201 (a 1.75 GiB array of mostly zero bytes)
-- after: 0
SELECT substring(CAST('Spark SQL' AS BINARY), -2147483648, 5);
-- before: the whole 9-byte input
-- after: empty
-- 2. raw JVM exception rather than a SparkThrowable
SELECT lpad(CAST('hi' AS BINARY), -1, CAST('??' AS BINARY));
-- before: java.lang.NegativeArraySizeException: -1
-- after: empty binary
-- 3. same, on the STRING side, at one specific length
SELECT lpad('hello', -2147483648, '??');
-- before: java.lang.ArithmeticException: integer overflow
-- after: empty string
```
The equivalent STRING expressions already returned `''` for the first two cases, so this is also a
STRING/BINARY consistency fix rather than new behaviour being invented. The STRING path was fixed
for this class of overflow in SPARK-32115, but `ByteArray.subStringSQL` (added by SPARK-28412) was
never brought in line, even though its comment says the offsets are computed "according to
UTF8String#subStringSQL". The pad overloads were added by SPARK-37047.
Beyond returning wrong results, the first case is a robustness concern: the allocation size is
driven by user-supplied arguments and is unrelated to the size of the input, so a modestly sized
query can allocate gigabytes and OOM the executor.
### Does this PR introduce _any_ user-facing change?
Yes, for the argument combinations described above, all of which are currently broken.
| Expression | Before | After |
|---|---|---|
| `substring(<binary>, -1207959552, -1207959552)` | 1879048201-byte array of zeros | empty binary |
| `substring(<binary>, -2147483648, 5)` | the whole input | empty binary |
| `lpad(<binary>, -1, <pad>)` | `NegativeArraySizeException` | empty binary |
| `rpad(<binary>, -1, <pad>)` | `NegativeArraySizeException` | empty binary |
| `lpad('hello', -2147483648, '??')` | `ArithmeticException` | empty string |
In every case the new result matches what the corresponding STRING expression already returned for
non-positive lengths. No behaviour changes for any in-range argument.
### How was this patch tested?
New unit tests:
- `ByteArraySuite.testSubStringSQL` and `ByteArraySuite.testPad` cover the positive, zero and
negative positions, the overflowing combinations above, and the null-input cases.
- `UTF8StringSuite.pad` gains the non-positive lengths including `Integer.MIN_VALUE`, for both an
empty and a non-empty padding pattern.
- `StringExpressionsSuite` covers the same combinations at the expression level for `Substring`,
`StringLPad` / `StringRPad` and `BinaryPad`. These go through `checkEvaluation`, so both the
interpreted and the codegen paths are exercised.
In addition, `ByteArray.subStringSQL` was checked against `UTF8String.substringSQL` as an oracle
over 3125 `(input, pos, len)` combinations built from boundary values (`Integer.MIN_VALUE`,
`Integer.MAX_VALUE`, `+/-1207959552`, and small offsets around the input length). For pure-ASCII
inputs character indexing and byte indexing coincide, so the two must agree exactly. Current master
reports 239 mismatches; with this patch, 0.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)
Closes #57922 from SEPURI-SAI-KRISHNA/SPARK-58708-substring-pad-length.
Authored-by: Sepuri Sai Krishna <sa...@gmail.com>
Signed-off-by: Uros Bojanic <22...@users.noreply.github.com>
(cherry picked from commit 0c00f46fb47468eeae759bd0f70bb773ade05edb)
Signed-off-by: Uros Bojanic <22...@users.noreply.github.com>
---
.../org/apache/spark/unsafe/types/ByteArray.java | 29 ++++-----
.../org/apache/spark/unsafe/types/UTF8String.java | 10 ++++
.../apache/spark/unsafe/array/ByteArraySuite.java | 69 ++++++++++++++++++++++
.../apache/spark/unsafe/types/UTF8StringSuite.java | 9 +++
.../expressions/StringExpressionsSuite.scala | 47 +++++++++++++++
5 files changed, 150 insertions(+), 14 deletions(-)
diff --git a/common/unsafe/src/main/java/org/apache/spark/unsafe/types/ByteArray.java b/common/unsafe/src/main/java/org/apache/spark/unsafe/types/ByteArray.java
index f12408fb4931..abd79fe7c6c4 100644
--- a/common/unsafe/src/main/java/org/apache/spark/unsafe/types/ByteArray.java
+++ b/common/unsafe/src/main/java/org/apache/spark/unsafe/types/ByteArray.java
@@ -113,26 +113,24 @@ public final class ByteArray {
public static byte[] subStringSQL(byte[] bytes, int pos, int len) {
// This pos calculation is according to UTF8String#subStringSQL
- if (pos > bytes.length) {
- return EMPTY_BYTE;
- }
int start = 0;
- int end;
if (pos > 0) {
start = pos - 1;
} else if (pos < 0) {
start = bytes.length + pos;
}
- if ((bytes.length - start) < len) {
- end = bytes.length;
- } else {
- end = start + len;
- }
+ // Compute the end offset as a `long`, since `start + len` can overflow the `int` range,
+ // then clamp it to the length of the input. `start` must stay unclamped until the end
+ // offset has been computed, because a negative `start` shortens the result. Clamping the
+ // end offset is required for correctness and not just as a bounds check: `copyOfRange`
+ // accepts an end offset past the end of the input and zero-pads the remainder, so an
+ // unclamped value silently yields a longer result instead of failing.
+ final long end = Math.min((long) start + len, bytes.length);
start = Math.max(start, 0); // underflow
if (start >= end) {
return EMPTY_BYTE;
}
- return Arrays.copyOfRange(bytes, start, end);
+ return Arrays.copyOfRange(bytes, start, (int) end);
}
/**
@@ -193,6 +191,7 @@ public final class ByteArray {
// Helper method for implementing `lpad` and `rpad`.
// If the padding pattern's length is 0, return the first `len` bytes of the input byte
// sequence if it is longer than `len` bytes, or a copy of the byte sequence, otherwise.
+ // `len` is expected to be positive; both callers return early for non-positive lengths.
private static byte[] padWithEmptyPattern(byte[] bytes, int len) {
len = Math.min(bytes.length, len);
final byte[] result = new byte[len];
@@ -222,8 +221,9 @@ public final class ByteArray {
// are returned. Otherwise, the remaining missing bytes are filled in with the provided pattern.
public static byte[] lpad(byte[] bytes, int len, byte[] pad) {
if (bytes == null || pad == null) return null;
- // If the input length is 0, return the empty byte sequence.
- if (len == 0) return EMPTY_BYTE;
+ // If the requested length is not positive, return the empty byte sequence. This matches
+ // `UTF8String.lpad`, which returns the empty string for any non-positive length.
+ if (len <= 0) return EMPTY_BYTE;
// The padding pattern is empty.
if (pad.length == 0) return padWithEmptyPattern(bytes, len);
// The general case.
@@ -250,8 +250,9 @@ public final class ByteArray {
// are returned. Otherwise, the remaining missing bytes are filled in with the provided pattern.
public static byte[] rpad(byte[] bytes, int len, byte[] pad) {
if (bytes == null || pad == null) return null;
- // If the input length is 0, return the empty byte sequence.
- if (len == 0) return EMPTY_BYTE;
+ // If the requested length is not positive, return the empty byte sequence. This matches
+ // `UTF8String.rpad`, which returns the empty string for any non-positive length.
+ if (len <= 0) return EMPTY_BYTE;
// The padding pattern is empty.
if (pad.length == 0) return padWithEmptyPattern(bytes, len);
// The general case.
diff --git a/common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java b/common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java
index a54c90c80a5d..3ff3027e7c30 100644
--- a/common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java
+++ b/common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java
@@ -1357,6 +1357,11 @@ public final class UTF8String implements Comparable<UTF8String>, Externalizable,
* ('hi', 1, '??') => 'h'
*/
public UTF8String rpad(int len, UTF8String pad) {
+ // If the requested length is not positive, the result is empty. This is also what the
+ // `spaces <= 0` branch below computes, but it has to be checked up front: for
+ // `len == Integer.MIN_VALUE` the subtraction wraps to a positive value and the padding
+ // branch is taken instead.
+ if (len <= 0) return EMPTY_UTF8;
int spaces = len - this.numChars(); // number of char need to pad
if (spaces <= 0 || pad.numBytes() == 0) {
// no padding at all, return the substring of the current string
@@ -1391,6 +1396,11 @@ public final class UTF8String implements Comparable<UTF8String>, Externalizable,
* ('hi', 1, '??') => 'h'
*/
public UTF8String lpad(int len, UTF8String pad) {
+ // If the requested length is not positive, the result is empty. This is also what the
+ // `spaces <= 0` branch below computes, but it has to be checked up front: for
+ // `len == Integer.MIN_VALUE` the subtraction wraps to a positive value and the padding
+ // branch is taken instead.
+ if (len <= 0) return EMPTY_UTF8;
int spaces = len - this.numChars(); // number of char need to pad
if (spaces <= 0 || pad.numBytes() == 0) {
// no padding at all, return the substring of the current string
diff --git a/common/unsafe/src/test/java/org/apache/spark/unsafe/array/ByteArraySuite.java b/common/unsafe/src/test/java/org/apache/spark/unsafe/array/ByteArraySuite.java
index 5e221b4e359d..f2dcacb56249 100644
--- a/common/unsafe/src/test/java/org/apache/spark/unsafe/array/ByteArraySuite.java
+++ b/common/unsafe/src/test/java/org/apache/spark/unsafe/array/ByteArraySuite.java
@@ -122,4 +122,73 @@ public class ByteArraySuite {
byte[] result4 = ByteArray.concatWS(separator, x4, y4);
Assertions.assertArrayEquals(null, result4);
}
+
+ @Test
+ public void testSubStringSQL() {
+ byte[] bytes = new byte[]{(byte) 1, (byte) 2, (byte) 3, (byte) 4, (byte) 5};
+ byte[] empty = new byte[0];
+
+ // Positive, zero and negative positions.
+ Assertions.assertArrayEquals(new byte[]{(byte) 2, (byte) 3},
+ ByteArray.subStringSQL(bytes, 2, 2));
+ Assertions.assertArrayEquals(new byte[]{(byte) 1, (byte) 2},
+ ByteArray.subStringSQL(bytes, 0, 2));
+ Assertions.assertArrayEquals(new byte[]{(byte) 4, (byte) 5},
+ ByteArray.subStringSQL(bytes, -2, 2));
+
+ // A position past the end of the input, and a length that runs past it.
+ Assertions.assertArrayEquals(empty, ByteArray.subStringSQL(bytes, 6, 2));
+ Assertions.assertArrayEquals(new byte[]{(byte) 4, (byte) 5},
+ ByteArray.subStringSQL(bytes, 4, 100));
+
+ // A non-positive length yields the empty byte sequence.
+ Assertions.assertArrayEquals(empty, ByteArray.subStringSQL(bytes, 2, 0));
+ Assertions.assertArrayEquals(empty, ByteArray.subStringSQL(bytes, 2, -1));
+
+ // SPARK-58708: `start + len` must be computed without overflowing the `int` range.
+ // Before the fix these returned a byte sequence longer than the input, zero-padded by
+ // `Arrays.copyOfRange`, instead of the empty byte sequence.
+ Assertions.assertArrayEquals(empty,
+ ByteArray.subStringSQL(bytes, -1207959552, -1207959552));
+ Assertions.assertArrayEquals(empty,
+ ByteArray.subStringSQL(bytes, -2147483647, Integer.MIN_VALUE));
+ Assertions.assertArrayEquals(empty,
+ ByteArray.subStringSQL(bytes, Integer.MIN_VALUE, 5));
+
+ // The same offsets, in combinations that do have a non-empty result.
+ Assertions.assertArrayEquals(bytes, ByteArray.subStringSQL(bytes, 1, Integer.MAX_VALUE));
+ Assertions.assertArrayEquals(new byte[]{(byte) 1, (byte) 2, (byte) 3, (byte) 4},
+ ByteArray.subStringSQL(bytes, Integer.MIN_VALUE, Integer.MAX_VALUE));
+ }
+
+ @Test
+ public void testPad() {
+ byte[] bytes = new byte[]{(byte) 1, (byte) 2};
+ byte[] pad = new byte[]{(byte) 3, (byte) 4};
+ byte[] emptyPad = new byte[0];
+ byte[] empty = new byte[0];
+
+ // Sanity: positive lengths are unaffected.
+ Assertions.assertArrayEquals(new byte[]{(byte) 3, (byte) 4, (byte) 3, (byte) 1, (byte) 2},
+ ByteArray.lpad(bytes, 5, pad));
+ Assertions.assertArrayEquals(new byte[]{(byte) 1, (byte) 2, (byte) 3, (byte) 4, (byte) 3},
+ ByteArray.rpad(bytes, 5, pad));
+ Assertions.assertArrayEquals(new byte[]{(byte) 1}, ByteArray.lpad(bytes, 1, pad));
+ Assertions.assertArrayEquals(new byte[]{(byte) 1}, ByteArray.rpad(bytes, 1, pad));
+
+ // SPARK-58708: a non-positive length yields the empty byte sequence, matching
+ // `UTF8String.lpad` and `UTF8String.rpad`. Before the fix a negative length reached
+ // `new byte[len]` and threw `NegativeArraySizeException`.
+ for (int len : new int[]{0, -1, -100, Integer.MIN_VALUE}) {
+ Assertions.assertArrayEquals(empty, ByteArray.lpad(bytes, len, pad));
+ Assertions.assertArrayEquals(empty, ByteArray.rpad(bytes, len, pad));
+ // The empty-padding-pattern path allocates separately and must be guarded too.
+ Assertions.assertArrayEquals(empty, ByteArray.lpad(bytes, len, emptyPad));
+ Assertions.assertArrayEquals(empty, ByteArray.rpad(bytes, len, emptyPad));
+ }
+
+ // A null input or padding pattern still yields null.
+ Assertions.assertArrayEquals(null, ByteArray.lpad(null, -1, pad));
+ Assertions.assertArrayEquals(null, ByteArray.rpad(bytes, -1, null));
+ }
}
diff --git a/common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java b/common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java
index e2222e1d0639..36a649fd83b4 100644
--- a/common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java
+++ b/common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java
@@ -404,6 +404,15 @@ public class UTF8StringSuite {
assertEquals(fromString("数据砖头"), fromString("数据砖头").rpad(5, EMPTY_UTF8));
assertEquals(fromString("数据砖"), fromString("数据砖头").rpad(3, EMPTY_UTF8));
assertEquals(EMPTY_UTF8, EMPTY_UTF8.rpad(3, EMPTY_UTF8));
+
+ // SPARK-58708: `len - numChars()` wraps for len == Integer.MIN_VALUE, which used to send
+ // a non-positive length down the padding branch and fail with an ArithmeticException.
+ for (int len : new int[]{0, -1, -100, Integer.MIN_VALUE}) {
+ assertEquals(EMPTY_UTF8, fromString("hello").lpad(len, fromString("??")));
+ assertEquals(EMPTY_UTF8, fromString("hello").rpad(len, fromString("??")));
+ assertEquals(EMPTY_UTF8, fromString("hello").lpad(len, EMPTY_UTF8));
+ assertEquals(EMPTY_UTF8, fromString("hello").rpad(len, EMPTY_UTF8));
+ }
}
@Test
diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/StringExpressionsSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/StringExpressionsSuite.scala
index 1687d614cc5e..583c9a2938c8 100644
--- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/StringExpressionsSuite.scala
+++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/StringExpressionsSuite.scala
@@ -329,6 +329,15 @@ class StringExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
checkEvaluation(Substring(bytes, -4, 2), Array[Byte](1, 2))
checkEvaluation(Substring(bytes, -5, 2), Array[Byte](1))
checkEvaluation(Substring(bytes, -8, 2), Array.empty[Byte])
+
+ // SPARK-58708: the end offset must be computed without overflowing, so that a binary
+ // input behaves like the string input tested above. Before the fix these returned a
+ // byte array much longer than the input, zero-padded by `Arrays.copyOfRange`.
+ checkEvaluation(Substring(bytes, -1207959552, -1207959552), Array.empty[Byte])
+ checkEvaluation(Substring(bytes, -2147483647, Int.MinValue), Array.empty[Byte])
+ checkEvaluation(Substring(bytes, Int.MinValue, 2), Array.empty[Byte])
+ checkEvaluation(Substring(bytes, 1, Int.MaxValue), Array[Byte](1, 2, 3, 4))
+ checkEvaluation(Substring(bytes, Int.MinValue, Int.MaxValue), Array[Byte](1, 2, 3))
}
test("string substring_index function") {
@@ -1044,6 +1053,44 @@ class StringExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
GenerateUnsafeProjection.generate(StringRPad(Literal("\"quote"), s2, Literal("\"quote")) :: Nil)
checkEvaluation(StringRPad(Literal("hi"), Literal(5)), "hi ")
checkEvaluation(StringRPad(Literal("hi"), Literal(1)), "h")
+
+ // SPARK-58708: a non-positive length returns the empty value rather than reaching the
+ // allocation. The string overloads already did; the binary ones threw
+ // NegativeArraySizeException, which is not a SparkThrowable.
+ val bin = Literal(Array[Byte](1, 2))
+ val binPad = Literal(Array[Byte](3, 4))
+ val emptyBinPad = Literal(Array[Byte]())
+ Seq(0, -1, -100, Int.MinValue).foreach { len =>
+ checkEvaluation(StringLPad(Literal("hi"), Literal(len), Literal("??")), "")
+ checkEvaluation(StringRPad(Literal("hi"), Literal(len), Literal("??")), "")
+ checkEvaluation(BinaryPad("lpad", bin, Literal(len), binPad), Array.empty[Byte])
+ checkEvaluation(BinaryPad("rpad", bin, Literal(len), binPad), Array.empty[Byte])
+ checkEvaluation(BinaryPad("lpad", bin, Literal(len), emptyBinPad), Array.empty[Byte])
+ checkEvaluation(BinaryPad("rpad", bin, Literal(len), emptyBinPad), Array.empty[Byte])
+ }
+ }
+
+ test("SPARK-58708: LPAD/RPAD length handling is collation-independent") {
+ // `StringLPad` and `StringRPad` call `UTF8String.lpad` and `UTF8String.rpad` directly.
+ // Neither dispatches on collation, and `CollationSupport` defines no collation-aware
+ // pad, so the collation only reaches the result type. The non-positive lengths fixed
+ // above therefore behave identically under every collation.
+ Seq("UTF8_BINARY", "UTF8_LCASE", "UNICODE", "UNICODE_CI").foreach { collation =>
+ val st = StringType(collation)
+ val str = Literal.create("hi", st)
+ val pad = Literal.create("??", st)
+ Seq(0, -1, -100, Int.MinValue).foreach { len =>
+ checkEvaluation(StringLPad(str, Literal(len), pad), "")
+ checkEvaluation(StringRPad(str, Literal(len), pad), "")
+ }
+ // Positive lengths are unaffected too, and the case of the input is preserved even
+ // under the case-insensitive collations.
+ checkEvaluation(StringLPad(str, Literal(5), pad), "???hi")
+ checkEvaluation(StringRPad(str, Literal(5), pad), "hi???")
+ val mixed = Literal.create("Hi", st)
+ checkEvaluation(StringLPad(mixed, Literal(2), pad), "Hi")
+ checkEvaluation(StringRPad(mixed, Literal(1), pad), "H")
+ }
}
test("PadExpressionBuilderBase") {
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@spark.apache.org
For additional commands, e-mail: commits-help@spark.apache.org