A few small tweaks to VectorUtil#findNextGEQ #13972
Conversation
gsmiller
commented
Nov 1, 2024
gsmiller
commented
- Rearrange/rename the parameters to be more idiomatic (e.g., follow conventions of Arrays#... methods)
- Add assert to ensure expected sortedness we may rely on in the future (so we're not trappy)
- Migrate PostingsReader to call VectorUtil instead of VectorUtilSupport (so it benefits from the common assert)
1. Rearrange/rename the parameters to be more idiomatic (e.g., follow conventions of Arrays#... methods) 2. Add assert to ensure expected sortedness we may rely on in the future (so we're not trappy) 3. Migrate PostingsReader to call VectorUtil instead of VectorUtilSupport (so it benefits from the common assert)
| @@ -1755,13 +1755,13 @@ static long readVLong15(DataInput in) throws IOException { | |||
| } | |||
| } | |||
|
|
|||
| private static int findNextGEQ(long[] buffer, int length, long target, int from) { | |||
| for (int i = from; i < length; ++i) { | |||
| private static int findNextGEQ(long[] buffer, long target, int from, int to) { | |||
There was a problem hiding this comment.
Made this change for consistency but don't have any strong feelings about it. If there's a preference to just leave it be, I'm fine reverting this file.
| @@ -601,7 +599,7 @@ public int advance(int target) throws IOException { | |||
| } | |||
| } | |||
|
|
|||
| int next = VECTOR_SUPPORT.findNextGEQ(docBuffer, docBufferSize, target, docBufferUpto); | |||
| int next = VectorUtil.findNextGEQ(docBuffer, target, docBufferUpto, docBufferSize); | |||
There was a problem hiding this comment.
@jpountz I made this change so that everything goes through VectorUtil and the assert. It also seemed a little more consistent with how we leverage VectorUtil vs. VectorUtilSupport elsewhere, but I'm not sure if you had a good reason for writing it the way you did. If there's a performance (or other) consideration here, I'd be happy to understand that better.
There was a problem hiding this comment.
I did it this way after doing something similar with PostingDecodingUtil, which didn't really belong to VectorUtil. I agree it's nicer the way you did it, I would not expect performance implications if inlining works properly, nightly benchmarks will tell us.
| @@ -601,7 +599,7 @@ public int advance(int target) throws IOException { | |||
| } | |||
| } | |||
|
|
|||
| int next = VECTOR_SUPPORT.findNextGEQ(docBuffer, docBufferSize, target, docBufferUpto); | |||
| int next = VectorUtil.findNextGEQ(docBuffer, target, docBufferUpto, docBufferSize); | |||
There was a problem hiding this comment.
I did it this way after doing something similar with PostingDecodingUtil, which didn't really belong to VectorUtil. I agree it's nicer the way you did it, I would not expect performance implications if inlining works properly, nightly benchmarks will tell us.
1. Rearrange/rename the parameters to be more idiomatic (e.g., follow conventions of Arrays#... methods) 2. Add assert to ensure expected sortedness we may rely on in the future (so we're not trappy) 3. Migrate PostingsReader to call VectorUtil instead of VectorUtilSupport (so it benefits from the common assert)
