This is an automated email from the ASF dual-hosted git repository.
uros-b pushed a commit to branch branch-4.x
in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/branch-4.x by this push:
new 356fbc459272 [SPARK-57659][CORE] Fix integer/long overflow in
ArrayWrappers numeric array key comparators
356fbc459272 is described below
commit 356fbc459272f76ec3148155da0be95431022bde
Author: YangJie <[email protected]>
AuthorDate: Thu Jun 25 00:15:44 2026 +0200
[SPARK-57659][CORE] Fix integer/long overflow in ArrayWrappers numeric
array key comparators
### What changes were proposed in this pull request?
`ComparableIntArray` and `ComparableLongArray` in `ArrayWrappers` compared
keys by subtracting elements (`array[i] - other.array[i]`). That subtraction
overflows for large opposite-sign values and flips the sign of the result.
Comparing `Integer.MIN_VALUE` with `Integer.MAX_VALUE` is enough to trigger it
for int, and `Long.MAX_VALUE - Long.MIN_VALUE` wraps around to `-1` for long.
The long version's `diff > 0 ? 1 : -1` looked like it dealt with this, but it
only kept the long differen [...]
### Why are the changes needed?
These wrappers order keys and index values in `InMemoryStore`.
`AppStatusStore` uses `int[]` keys (stage id and attempt id), so the int
comparator runs for real, but those ids are small so the overflow never
surfaced. No in-tree entity uses `long[]` keys today. The bug is real but
latent, and the fix keeps the ordering correct if larger values ever flow
through.
### Does this PR introduce _any_ user-facing change?
No. It only fixes orderings that were already wrong.
### How was this patch tested?
Added two cases to `ArrayWrappersSuite`, one per comparator, covering
`MIN`/`MAX` and other large opposite-sign values in both directions. To confirm
they actually guard the bug, I reverted each fix on its own and checked that
only that comparator's new test failed.
### Was this patch authored or co-authored using generative AI tooling?
No
Closes #56731 from LuciferYang/SPARK-kvstore-intarray-overflow.
Authored-by: YangJie <[email protected]>
Signed-off-by: Uros Bojanic <[email protected]>
(cherry picked from commit 9da9020ebc092b8d5d18eb3d3066f50f0ca70f11)
Signed-off-by: Uros Bojanic <[email protected]>
---
.../apache/spark/util/kvstore/ArrayWrappers.java | 14 +++++---
.../spark/util/kvstore/ArrayWrappersSuite.java | 40 ++++++++++++++++++++++
2 files changed, 49 insertions(+), 5 deletions(-)
diff --git
a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/ArrayWrappers.java
b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/ArrayWrappers.java
index a9d6784805f6..477fbdb0d34c 100644
---
a/common/kvstore/src/main/java/org/apache/spark/util/kvstore/ArrayWrappers.java
+++
b/common/kvstore/src/main/java/org/apache/spark/util/kvstore/ArrayWrappers.java
@@ -83,13 +83,15 @@ class ArrayWrappers {
public int compareTo(ComparableIntArray other) {
int len = Math.min(array.length, other.array.length);
for (int i = 0; i < len; i++) {
- int diff = array[i] - other.array[i];
+ // Use Integer.compare; subtraction overflows for large opposite-sign
values
+ // (e.g. Integer.MIN_VALUE vs Integer.MAX_VALUE) and returns the wrong
ordering.
+ int diff = Integer.compare(array[i], other.array[i]);
if (diff != 0) {
return diff;
}
}
- return array.length - other.array.length;
+ return Integer.compare(array.length, other.array.length);
}
}
@@ -122,13 +124,15 @@ class ArrayWrappers {
public int compareTo(ComparableLongArray other) {
int len = Math.min(array.length, other.array.length);
for (int i = 0; i < len; i++) {
- long diff = array[i] - other.array[i];
+ // Use Long.compare; subtraction overflows for large opposite-sign
values
+ // (e.g. Long.MIN_VALUE vs Long.MAX_VALUE) and returns the wrong
ordering.
+ int diff = Long.compare(array[i], other.array[i]);
if (diff != 0) {
- return diff > 0 ? 1 : -1;
+ return diff;
}
}
- return array.length - other.array.length;
+ return Integer.compare(array.length, other.array.length);
}
}
diff --git
a/common/kvstore/src/test/java/org/apache/spark/util/kvstore/ArrayWrappersSuite.java
b/common/kvstore/src/test/java/org/apache/spark/util/kvstore/ArrayWrappersSuite.java
index 54bf5a2e60fd..031d844584de 100644
---
a/common/kvstore/src/test/java/org/apache/spark/util/kvstore/ArrayWrappersSuite.java
+++
b/common/kvstore/src/test/java/org/apache/spark/util/kvstore/ArrayWrappersSuite.java
@@ -56,4 +56,44 @@ public class ArrayWrappersSuite {
assertTrue(ArrayWrappers.forArray(s1).compareTo(ArrayWrappers.forArray(s2))
> 0);
}
+ @Test
+ public void testIntArrayCompareNoOverflow() {
+ // Large opposite-sign values overflow a subtraction-based comparison, so
these
+ // assertions fail unless compareTo uses Integer.compare.
+ Comparable<Object> min = ArrayWrappers.forArray(new int[] {
Integer.MIN_VALUE });
+ Comparable<Object> max = ArrayWrappers.forArray(new int[] {
Integer.MAX_VALUE });
+ Comparable<Object> negOne = ArrayWrappers.forArray(new int[] { -1 });
+
+ assertTrue(min.compareTo(max) < 0);
+ assertTrue(max.compareTo(min) > 0);
+ assertTrue(max.compareTo(negOne) > 0);
+ assertTrue(negOne.compareTo(max) < 0);
+
+ // Ordering must stay correct beyond the first element as well.
+ Comparable<Object> a = ArrayWrappers.forArray(new int[] { 0,
Integer.MIN_VALUE });
+ Comparable<Object> b = ArrayWrappers.forArray(new int[] { 0,
Integer.MAX_VALUE });
+ assertTrue(a.compareTo(b) < 0);
+ assertTrue(b.compareTo(a) > 0);
+ }
+
+ @Test
+ public void testLongArrayCompareNoOverflow() {
+ // Large opposite-sign values overflow a subtraction-based comparison, so
these
+ // assertions fail unless compareTo uses Long.compare.
+ Comparable<Object> min = ArrayWrappers.forArray(new long[] {
Long.MIN_VALUE });
+ Comparable<Object> max = ArrayWrappers.forArray(new long[] {
Long.MAX_VALUE });
+ Comparable<Object> negOne = ArrayWrappers.forArray(new long[] { -1L });
+
+ assertTrue(min.compareTo(max) < 0);
+ assertTrue(max.compareTo(min) > 0);
+ assertTrue(max.compareTo(negOne) > 0);
+ assertTrue(negOne.compareTo(max) < 0);
+
+ // Ordering must stay correct beyond the first element as well.
+ Comparable<Object> a = ArrayWrappers.forArray(new long[] { 0L,
Long.MIN_VALUE });
+ Comparable<Object> b = ArrayWrappers.forArray(new long[] { 0L,
Long.MAX_VALUE });
+ assertTrue(a.compareTo(b) < 0);
+ assertTrue(b.compareTo(a) > 0);
+ }
+
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
