[SPARK-57659][CORE] Fix integer/long overflow in ArrayWrappers numeric array key comparators#56731
Open
LuciferYang wants to merge 1 commit into
Open
[SPARK-57659][CORE] Fix integer/long overflow in ArrayWrappers numeric array key comparators#56731LuciferYang wants to merge 1 commit into
LuciferYang wants to merge 1 commit into
Conversation
…c 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 difference from being truncated to int; the overflow had already happened by then. Both comparators now use `Integer.compare` / `Long.compare`, which can't overflow. `ComparableByteArray` is left as is, since bytes promote into a small int range and its subtraction can't overflow. ### 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
LuciferYang
force-pushed
the
SPARK-kvstore-intarray-overflow
branch
from
22ebdd9 to
4a03bd6
Compare
uros-b
reviewed
Jun 24, 2026
| } | ||
| } | ||
|
|
||
| return array.length - other.array.length; |
Member
There was a problem hiding this comment.
Nit: here we still use subtraction. Should we switch to Integer.compare for consistency?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
ComparableIntArrayandComparableLongArrayinArrayWrapperscompared keys by subtracting elements (array[i] - other.array[i]). That subtraction overflows for large opposite-sign values and flips the sign of the result. ComparingInteger.MIN_VALUEwithInteger.MAX_VALUEis enough to trigger it for int, andLong.MAX_VALUE - Long.MIN_VALUEwraps around to-1for long. The long version'sdiff > 0 ? 1 : -1looked like it dealt with this, but it only kept the long difference from being truncated to int; the overflow had already happened by then. Both comparators now useInteger.compare/Long.compare, which can't overflow.ComparableByteArrayis left as is, since bytes promote into a small int range and its subtraction can't overflow.Why are the changes needed?
These wrappers order keys and index values in
InMemoryStore.AppStatusStoreusesint[]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 useslong[]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, coveringMIN/MAXand 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