You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2026/06/24 07:19:50 UTC
[PR] [SPARK-57659][CORE] Fix integer/long overflow in ArrayWrappers numeric array key comparators [spark]
LuciferYang opened a new pull request, #56731:
URL: https://github.com/apache/spark/pull/56731
### 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` keeps the subtraction because bytes promote into a small int range so it can't overflow, and I left a comment saying as much.
### 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
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57659][CORE] Fix integer/long overflow in ArrayWrappers numeric array key comparators [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b closed pull request #56731: [SPARK-57659][CORE] Fix integer/long overflow in ArrayWrappers numeric array key comparators
URL: https://github.com/apache/spark/pull/56731
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57659][CORE] Fix integer/long overflow in ArrayWrappers numeric array key comparators [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #56731:
URL: https://github.com/apache/spark/pull/56731#discussion_r3467244633
##########
common/kvstore/src/main/java/org/apache/spark/util/kvstore/ArrayWrappers.java:
##########
Review Comment:
Good point. I switched the length tie-break to `Integer.compare` in both `ComparableIntArray` and `ComparableLongArray`, so each method is consistent with its element comparison. (The length subtraction was actually overflow-safe since lengths are non-negative, but `Integer.compare` reads better alongside the rest.) I left `ComparableByteArray` and `ComparableObjectArray` as they are: `ComparableByteArray` compares elements by subtraction too and is overflow-safe for the byte range, and `ComparableObjectArray` is outside this PR's scope.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57659][CORE] Fix integer/long overflow in ArrayWrappers numeric array key comparators [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #56731:
URL: https://github.com/apache/spark/pull/56731#issuecomment-4794112303
Merging to master/4.x.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57659][CORE] Fix integer/long overflow in ArrayWrappers numeric array key comparators [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #56731:
URL: https://github.com/apache/spark/pull/56731#discussion_r3466541120
##########
common/kvstore/src/main/java/org/apache/spark/util/kvstore/ArrayWrappers.java:
##########
Review Comment:
Nit: here we still use subtraction. Should we switch to Integer.compare for consistency?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57659][CORE] Fix integer/long overflow in ArrayWrappers numeric array key comparators [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #56731:
URL: https://github.com/apache/spark/pull/56731#issuecomment-4795464497
Thank you @uros-b
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org