Feature/previous next values

[bearrito]

This will address - #380

There a few edge cases I'm not sure about. Namely ones where the the next or previous values would lie completely outside.

[Oppen]

I think it looks good for the most part. I have a few suggestions and I'd like for the docs to be more clear about the meaning. I had to do some guess work to understand what the functionality achieves, ideally users shouldn't have to do that.

[Oppen]

Reminder for myself: find a test case for the bugs I pointed out before. The conditions are that:

1. target is within bounds;
2. target is not in array;
3. array exhausts the loop and falls to the default return point.

[lemire]

running tests

[Oppen]

Running tests.

[bearrito]

Looks like these passed. I still need to implement previousAbsentValue for array containers. That should be straightforward.

[lemire]

Running tests.

[bearrito]

I'll look at the failures. They are on my end.

[Oppen]

Running again.

[bearrito]

Tests pass. I don't think I can think of how to apply any more test pressure.

I did have a question about the overall design. I more or less referred to the Java implementation. I had a question about what was done there and how users use this feature. Why do we not for a given container, especially for absent value return values that are in the keyspace of a given key.

Example: For a run container with HOB X, we can calculate the range of of values that could be under X, call that the image of X. Today if the query/target is less than min(Union(ranges)) we return -1 meaning out of bound. Why would we not min(Union(ranges))-1 if that value was in the image of X.

What are the users query patterns that make that not useful?

[lemire]

For a run container with HOB X, we can calculate the range of of values that could be under X, call that the image of X. Today if the query/target is less than min(Union(ranges)) we return -1 meaning out of bound. Why would we not min(Union(ranges))-1 if that value was in the image of X.

Can you elaborate?

What code do you propose changing?

[lemire]

I have reopened the issue as I think it was closed by mistake.

Let me go back to the Java functionality...

  /**
   * Returns the first value equal to or larger than the provided value
   * (interpreted as an unsigned integer). If no such
   * bit exists then {@code -1} is returned. It is not necessarily a
   * computationally effective way to iterate through the values.
   *
   * @param  fromValue the lower bound (inclusive)
   * @return the smallest value larger than or equal to the specified value,
   *       or {@code -1} if there is no such value
   */
  long nextValue(int fromValue);
  
  /**
   * Returns the first value less than or equal to the provided value
   * (interpreted as an unsigned integer). If no such
   * bit exists then {@code -1} is returned. It is not an efficient
   * way to iterate through the values backwards.
   *
   * @param  fromValue the upper bound (inclusive)
   * @return the largest value less than or equal to the specified value,
   *       or {@code -1} if there is no such value
   */
  long previousValue(int fromValue);

The current PR only addresses the underlying containers but does not yet exposes the functionality to the users.

In Java, we return a -1 (long) when no value is present.

[bearrito]

@lemire yeah I closed it on purpose, so it would not accidentally be merged.

I noticed that as well. I'm implementing that functionality.

I had a few other edge-case bugs. What I did for those was to port all the java tests over and confirm that we match the java tests.

[lemire]

@bearrito Your work prompted me to do some work in Javaland:

RoaringBitmap/RoaringBitmap#720

... as it was ill-defined in some edge cases.

[bearrito]

@lemire

As mentioned I ported the Java tests line for line over for runs and arrays. I didn't really understand the Java fluent data builder used by bitmaps was doing so I couldn't port for bitmaps. I'm happy with the port, that's a good safety net. I picked that last release and included doc links to the corresponding java tests.

[bearrito]

@lemire
My fuzz tests are failing. I think I have an overflow issue. But they are passing on my machine

--- PASS: TestNextAndPreviousValue (0.05s)
    --- PASS: TestNextAndPreviousValue/randomized (0.05s)

Is this an architecture issue? I'll look at it more.

[lemire]

Suggested change

	var responseBit int
	var responseBit int64

[lemire]

Suggested change

	responseBit = int(bit)
	responseBit = int64(bit)

[lemire]

Suggested change

	nextValue = int(combineLoHi32(uint32(responseBit), uint32(containerKey)))
	nextValue = int64(combineLoHi32(uint32(responseBit), uint32(containerKey)))

[lemire]

@bearrito I have marked some suggested changes. The problem is that there is a hidden assumption that int spans 64 bits, but it does not. It is either 32 bits or 64 bits. So you should only use int when it is ok if int == int32.

Generally, it is safer to use int64 than int.

[lemire]

Is this an architecture issue? I'll look at it more.

Yes. It is. On a 32-bit platform they would fail.

Try setting GOARCH to 386 or other 32-bit system.

[bearrito]

Thanks. I'm generally poor at low-level bit-thinking/machine sympathy (partly why I wanted to contribute to this project)
I'll correct it.

[lemire]

Thanks. I'm generally poor at low-level bit-thinking/machine sympathy (partly why I wanted to contribute to this project)
I'll correct it.

It is not at all trivial that int has a different size depending on the system. :-)

[bearrito]

Updated. Tested locally by toggling GOARCH

[lemire]

Looks good to me. If you recommend merging, we will do so.

[bearrito]

I'm satisfied, please merge.