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/22 08:31:39 UTC
[PR] [SPARK-57597][CORE] Guard ByteUnit.toBytes against long overflow [spark]
LuciferYang opened a new pull request, #56655:
URL: https://github.com/apache/spark/pull/56655
### What changes were proposed in this pull request?
`ByteUnit.toBytes` checked for a negative argument and then returned `d * multiplier` directly, with no guard against the multiplication overflowing a `long`. For the larger units this can wrap silently: `PiB.toBytes(8192)` returns `Long.MIN_VALUE` instead of reporting an out-of-range value. The sibling `convertTo` already guards the same multiplication with `Long.MAX_VALUE / ratio < d`, so this PR adds the equivalent check to `toBytes`: an out-of-range conversion now throws `IllegalArgumentException` rather than returning a wrapped result.
It also adds `ByteUnitSuite`, which the module did not have, covering the per-unit conversions (including zero), negative input, the overflow case, and the largest value that still fits in a `long`.
### Why are the changes needed?
A size conversion that silently wraps to a negative number is worse than one that fails loudly, and `toBytes` was the only conversion method on this enum without the overflow guard that `convertTo` already has. Making the two consistent removes a latent correctness gap on a public utility.
### Does this PR introduce _any_ user-facing change?
No. Values that fit in a `long` convert exactly as before; only an input large enough to overflow now throws `IllegalArgumentException` instead of returning a wrapped value.
### How was this patch tested?
Added `ByteUnitSuite`. `build/sbt 'common-utils-java/testOnly *ByteUnitSuite'` passes.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)
--
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-57597][CORE] Guard ByteUnit.toBytes against long overflow [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b closed pull request #56655: [SPARK-57597][CORE] Guard ByteUnit.toBytes against long overflow
URL: https://github.com/apache/spark/pull/56655
--
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-57597][CORE] Guard ByteUnit.toBytes against long overflow [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #56655:
URL: https://github.com/apache/spark/pull/56655#discussion_r3452655701
##########
common/utils-java/src/main/java/org/apache/spark/network/util/ByteUnit.java:
##########
@@ -54,6 +54,10 @@ public long toBytes(long d) {
if (d < 0) {
throw new IllegalArgumentException("Negative size value. Size must be positive: " + d);
}
+ if (Long.MAX_VALUE / multiplier < d) {
+ throw new IllegalArgumentException("Conversion of " + d + " " + name()
Review Comment:
Good call -- extracted the shared check into a private `checkedMultiply(d, factor)` that both `convertTo` and `toBytes` use now.
##########
common/utils-java/src/main/java/org/apache/spark/network/util/ByteUnit.java:
##########
@@ -54,6 +54,10 @@ public long toBytes(long d) {
if (d < 0) {
throw new IllegalArgumentException("Negative size value. Size must be positive: " + d);
}
+ if (Long.MAX_VALUE / multiplier < d) {
+ throw new IllegalArgumentException("Conversion of " + d + " " + name()
+ + " to bytes exceeds Long.MAX_VALUE");
Review Comment:
Done -- `checkedMultiply` emits the same hint. It applies to `toBytes` too: bytes is the smallest unit, so the fix for an overflow is to convert to a larger unit (e.g. `toMiB`) instead, as the existing `UtilsSuite` overflow test shows (`PiB.toMiB(1073741824)` doesn't overflow).
--
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-57597][CORE] Guard ByteUnit.toBytes against long overflow [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #56655:
URL: https://github.com/apache/spark/pull/56655#issuecomment-4781061036
Thank you @uros-b and @MaxGekk
--
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-57597][CORE] Guard ByteUnit.toBytes against long overflow [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #56655:
URL: https://github.com/apache/spark/pull/56655#discussion_r3452153110
##########
common/utils-java/src/main/java/org/apache/spark/network/util/ByteUnit.java:
##########
@@ -54,6 +54,10 @@ public long toBytes(long d) {
if (d < 0) {
throw new IllegalArgumentException("Negative size value. Size must be positive: " + d);
}
+ if (Long.MAX_VALUE / multiplier < d) {
+ throw new IllegalArgumentException("Conversion of " + d + " " + name()
Review Comment:
Nit: `toBytes` and `convertTo` now duplicate the overflow idiom; a shared helper could DRY it.
--
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-57597][CORE] Guard ByteUnit.toBytes against long overflow [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #56655:
URL: https://github.com/apache/spark/pull/56655#discussion_r3452655928
##########
common/utils-java/src/main/java/org/apache/spark/network/util/ByteUnit.java:
##########
@@ -54,6 +54,10 @@ public long toBytes(long d) {
if (d < 0) {
throw new IllegalArgumentException("Negative size value. Size must be positive: " + d);
}
+ if (Long.MAX_VALUE / multiplier < d) {
+ throw new IllegalArgumentException("Conversion of " + d + " " + name()
+ + " to bytes exceeds Long.MAX_VALUE");
Review Comment:
Done, `checkedMultiply` emits the same hint. It applies to `toBytes` too: bytes is the smallest unit, so the fix for an overflow is to convert to a larger unit (e.g. `toMiB`) instead, as the existing `UtilsSuite` overflow test shows (`PiB.toMiB(1073741824)` doesn't overflow).
##########
common/utils-java/src/main/java/org/apache/spark/network/util/ByteUnit.java:
##########
@@ -54,6 +54,10 @@ public long toBytes(long d) {
if (d < 0) {
throw new IllegalArgumentException("Negative size value. Size must be positive: " + d);
}
+ if (Long.MAX_VALUE / multiplier < d) {
+ throw new IllegalArgumentException("Conversion of " + d + " " + name()
Review Comment:
Good call, extracted the shared check into a private `checkedMultiply(d, factor)` that both `convertTo` and `toBytes` use now.
--
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-57597][CORE] Guard ByteUnit.toBytes against long overflow [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #56655:
URL: https://github.com/apache/spark/pull/56655#discussion_r3452158021
##########
common/utils-java/src/main/java/org/apache/spark/network/util/ByteUnit.java:
##########
@@ -54,6 +54,10 @@ public long toBytes(long d) {
if (d < 0) {
throw new IllegalArgumentException("Negative size value. Size must be positive: " + d);
}
+ if (Long.MAX_VALUE / multiplier < d) {
+ throw new IllegalArgumentException("Conversion of " + d + " " + name()
+ + " to bytes exceeds Long.MAX_VALUE");
Review Comment:
Nit: `convertTo` also gives a hint ("Try a larger unit"); could we give a brief hint here as well?
--
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