Skip to content

chore: LongRange Does Not Support Empty Range #1982

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ata-nas wants to merge 1 commit into
base: main
Choose a base branch
from
+44 −56
Open

chore: LongRange Does Not Support Empty Range #1982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 43 additions & 51 deletions ...k-node/spi-plugins/src/main/java/org/hiero/block/node/spi/historicalblocks/LongRange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,59 +7,41 @@

/**
* Contiguous range of long values, inclusive of start and end.
* Valid ranges must have start and end values between 0 and Long.MAX_VALUE-1 inclusive,
* with start less than or equal to end. This ensures that the size() method can correctly
* represent any valid range with a long value without risk of overflow.
* Valid ranges must have start and end values between 0 and Long.MAX_VALUE-1
* inclusive, with start less than or equal to end. This ensures that the size()
* method can correctly represent any valid range with a long value without risk
* of overflow.
*/
public record LongRange(long start, long end) implements Comparable<LongRange> {

/** Comparator for comparing LongRange objects by their start and end values. */
public static final Comparator<LongRange> COMPARATOR =
Comparator.comparingLong(LongRange::start).thenComparingLong(LongRange::end);

/**
* Creates a new LongRange with the specified start and end values.
*
* @param start the start value of the range (inclusive), must be between 0 and Long.MAX_VALUE-1
* @param end the end value of the range (inclusive), must be between 0 and Long.MAX_VALUE-1
* @throws IllegalArgumentException if start or end is negative or greater than Long.MAX_VALUE-1,
* or if start is greater than end
* @param start the start value of the range (inclusive), must be between 0
* and Long.MAX_VALUE-1
* @param end the end value of the range (inclusive), must be between 0 and
* Long.MAX_VALUE-1
* @throws IllegalArgumentException if start or end is negative or greater
* than Long.MAX_VALUE-1, or if start is greater than end
*/
public LongRange {
// Special case: allow both start and end to be -1 for clean state initialization
if (!(start == -1 && end == -1)) {
if (start < 0) {
throw new IllegalArgumentException("Range start must be non-negative: " + start);
}
if (end < 0) {
throw new IllegalArgumentException("Range end must be non-negative: " + end);
}
if (start > end) {
throw new IllegalArgumentException(
"Range start must be less than or equal to end: " + start + " > " + end);
}
if (end > Long.MAX_VALUE - 1) {
throw new IllegalArgumentException("Range end must be less than or equal to Long.MAX_VALUE-1: " + end);
}
if (start < 0) {
throw new IllegalArgumentException("LongRange start: %d must not be negative".formatted(start));
}
if (end < 0) {
throw new IllegalArgumentException("LongRange end: %d must not be negative".formatted(end));
}
if (end > Long.MAX_VALUE - 1) {
Copy link
Contributor

@jsync-swirlds jsync-swirlds Dec 18, 2025

Choose a reason for hiding this comment

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

Why the -1 here? Shouldn't we support a Long range that ends at Long.MAX_VALUE?
That would also eliminate the need to check here, as long cannot be greater than max vlaue.

Copy link
Contributor Author

@ata-nas ata-nas Dec 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I believe this is because java uses signed longs, but we use unsigned longs in the proto. It was firstly developed like so. In order to not overflow on the edge case, or because of the size() method, as explained there.

Copy link
Contributor

@jsync-swirlds jsync-swirlds Dec 18, 2025

Choose a reason for hiding this comment

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

I suspect that it's safe to use the actual max value now, it was probably always safe, because the unsigned usage in the proto just means we get negative values (in java) for proto values greater than Long.MAX_VALUE.

Overflow throws an exception in most cases; and even if it doesn't (e.g. someone uses bit manipulation instead of arithmetic functions) we just end up with a negative number (which will be rejected).

Copy link
Contributor Author

@ata-nas ata-nas Jan 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

So, this is all about the correctness of the size method then. As in order for it to be correct, it has to add a +1 on the difference between first and last. If range is 0 -> max long, then we will add a +1 to the max long, which will be problematic.

Copy link
Contributor

@jsync-swirlds jsync-swirlds Jan 5, 2026

Choose a reason for hiding this comment

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

Adding +1 to max long results in Long.MIN_VALUE, which should be easy to handle if necessary.
The net effect of this should still be correct.

Also, why is size adding +1? That seems like it's inviting an error sooner or later.

Copy link
Contributor Author

@ata-nas ata-nas Jan 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

The issue is that a range 0 -> long max (which is valid) indeed has a size of long max + 1 which will overflow in java. So there is really nothing we can do, we cannot return long max + 1.

P.S. based on your message, I thought we will have to support the unsigned longs (soon), nevertheless that we will not reach such values for a very long time, that is why I thought we are taking some action. But in that case, I guess it is ok to leave the current implementation as it will not have issues with the size method.

Copy link
Contributor

@jsync-swirlds jsync-swirlds Jan 6, 2026

Choose a reason for hiding this comment

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

I would have size() detect the overflow and return the negative (with appropriate documentation that this is an overflow) rather than having this check here that is more likely to become an error eventually.

It's not a critical issue, more a recommendation to put the edge case handling in the place that needs them rather than pushing them out to another element.

Copy link
Contributor Author

@ata-nas ata-nas Jan 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

We could do that. Documenting that size could return Long.MIN_VALUE in the case where we have the range 0 -> Long.MAX_VALUE. Still torn which is the better option. We then rely on the caller to actually be aware of this.

Copy link
Contributor

@jsync-swirlds jsync-swirlds Jan 6, 2026

Choose a reason for hiding this comment

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

Note, size could return any negative value, we're basically acknowledging (in javadoc) that size is returning a long that should be treated as unsigned if necessary, rather than setting an early limit on the permitted ranges.

Either is acceptable; feel free to resolve this comment when you've decided which approach to proceed with.

Copy link
Contributor Author

@ata-nas ata-nas Jan 13, 2026

Choose a reason for hiding this comment

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

Getting back to this after some break in order to see it from another perspective.

So, my fear is misuse. There is room for misuse in both cases. The way I see it is:

  1. Can we rely on the user to actually read the documentation and make sure the code actually takes Long.MIN_VALUE into account?

VS

  1. Can we rely on the user to read the documentation and know that the max we support is Long.MAX_VALUE -1, otherwise we throw?

The issue that is bugging me, is that if we migrate to scenario 1, i.e. change the size method to return a negative value and support Long.MAX_VALUE as the max range value, then we will also have to make amends in all places we currently use the LongRange to make sure we do not have the possibility for a misuse. That scares me, it sounds like a really dangerous thing to do for, possibly, no added benefit.

I am kind of leaning more towards preserving what we currently have, maybe adding some additional notes for clarification.

throw new IllegalArgumentException(
"LongRange end: %d must not be greater than Long.MAX_VALUE-1".formatted(end));
}
if (start > end) {
throw new IllegalArgumentException(
"LongRange start: %d must not be greater than end: %d".formatted(start, end));
}
}

/**
* Gets the start value of the range, inclusive.
*
* @return the start value of the range
*/
public long start() {
return start;
}

/**
* Gets the end value of the range, inclusive.
*
* @return the end value of the range
*/
public long end() {
return end;
}

/**
Expand All @@ -68,25 +50,34 @@ public long end() {
* @param value the value to check
* @return true if the range contains the value, false otherwise
*/
public boolean contains(long value) {
public boolean contains(final long value) {
return value >= start && value <= end;
}

/**
* Checks if the range contains another range specified by start and end values.
* Checks if the range contains another range specified by start and end
* values.
*
* @param start the start value of the range to check
* @param end the end value of the range to check
* @return true if the range contains the specified range, false otherwise
*/
public boolean contains(long start, long end) {
return start >= this.start && end <= this.end;
public boolean contains(final long start, final long end) {
if (start > end) {
// if the caller supplies invalid values, responsibility falls on
// them, we must return a response that makes sense based on input,
// in this case false, as no range can start after it ends
return false;
} else {
return start >= this.start && end <= this.end;
}
}

/**
* Gets the size of the range.
*
* @return the size of the range (number of elements), computed as end - start + 1
* @return the size of the range (number of elements),
* computed as end - start + 1
*/
public long size() {
return end - start + 1;
Expand All @@ -98,7 +89,7 @@ public long size() {
* @param other the other range to check
* @return true if the ranges overlap, false otherwise
*/
public boolean overlaps(LongRange other) {
public boolean overlaps(final LongRange other) {
return !(end < other.start() || start > other.end());
}

Expand All @@ -118,7 +109,7 @@ public boolean isAdjacent(LongRange other) {
* @param other the other range to merge with
* @return a new ImmutableLongRange representing the merged range
*/
public LongRange merge(LongRange other) {
public LongRange merge(final LongRange other) {
return new LongRange(Math.min(start, other.start()), Math.max(end, other.end()));
}

Expand All @@ -135,11 +126,11 @@ public LongStream stream() {
* Compares this range to another range.
*
* @param o the other range to compare to
* @return a negative integer, zero, or a positive integer as this range is less than, equal to, or greater than
* the specified range
* @return a negative integer, zero, or a positive integer as this range is
* less than, equal to, or greater than the specified range
*/
@Override
public int compareTo(@NonNull LongRange o) {
public int compareTo(@NonNull final LongRange o) {
return COMPARATOR.compare(this, o);
}

Expand All @@ -148,6 +139,7 @@ public int compareTo(@NonNull LongRange o) {
*
* @return a string representation of the range in the format "start->end"
*/
@NonNull
@Override
public String toString() {
return start + "->" + end;
Expand Down
6 changes: 1 addition & 5 deletions ...de/spi-plugins/src/test/java/org/hiero/block/node/spi/historicalblocks/LongRangeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,8 @@ void testConstructor() {
assertEquals(10, range3.start());
assertEquals(10, range3.end());

// Test special case for clean state initialization
final LongRange cleanStateRange = new LongRange(-1, -1);
assertEquals(-1, cleanStateRange.start());
assertEquals(-1, cleanStateRange.end());

// Test validation failures
assertThrows(IllegalArgumentException.class, () -> new LongRange(-1, -1));
assertThrows(IllegalArgumentException.class, () -> new LongRange(-1, 5));
assertThrows(IllegalArgumentException.class, () -> new LongRange(5, -1));
assertThrows(IllegalArgumentException.class, () -> new LongRange(6, 5));
Expand Down
Loading