Skip to content

[SPARK-57659][CORE] Fix ArrayWrappers int[]/long[] comparator overflow for opposite-sign values#56743

Closed
aviyehuda wants to merge 2 commits into
apache:masterfrom
aviyehuda:SPARK-57659
Closed

[SPARK-57659][CORE] Fix ArrayWrappers int[]/long[] comparator overflow for opposite-sign values#56743
aviyehuda wants to merge 2 commits into
apache:masterfrom
aviyehuda:SPARK-57659

Conversation

@aviyehuda

Copy link
Copy Markdown

What changes were proposed in this pull request?

ComparableIntArray and ComparableLongArray in ArrayWrappers now compare elements with Integer.compare and Long.compare instead of subtraction.

A unit test (testOppositeSignArrayKeyComparison) was added to cover MIN_VALUE vs MAX_VALUE for both int[] and long[].

Why are the changes needed?

ArrayWrappers is used as a key comparator in InMemoryStore for array-typed indices. The previous implementation compared elements via subtraction:

int diff = array[i] - other.array[i];   // int[]
long diff = array[i] - other.array[i];  // long[]

For large opposite-sign values (e.g. Integer.MIN_VALUE vs Integer.MAX_VALUE), subtraction overflows and can return the wrong sign, breaking sort order and map lookups. The long[] path tried to mitigate this with diff > 0 ? 1 : -1, but overflow can still flip the sign of diff, so the result is still wrong.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Added testOppositeSignArrayKeyComparison in ArrayWrappersSuite.

build/sbt 'kvstore/testOnly *ArrayWrappersSuite'
Both tests in the suite passed.

Was this patch authored or co-authored using generative AI tooling?

No.

@uros-b uros-b left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aviyehuda Please note that @LuciferYang was already working on this: #56731.

@uros-b

uros-b commented Jun 24, 2026

Copy link
Copy Markdown
Member

@aviyehuda Please note that @LuciferYang was already working on this: #56731.

I recommend closing the current PR as a duplicate of existing work.

@uros-b

uros-b commented Jun 25, 2026

Copy link
Copy Markdown
Member

Closing as duplicate of #56731.
Thank you @aviyehuda and @LuciferYang!

@uros-b uros-b closed this Jun 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants