Add BitSet#or opto and fixed some bugs#1
Add BitSet#or opto and fixed some bugs#1mdmarshmallow wants to merge 1 commit intozacharymorn:LUCENE-11915-PeekNextNonMatchingDocfrom
Conversation
| checkUnpositioned(iter); | ||
| for (int doc = iter.nextDoc(); doc != DocIdSetIterator.NO_MORE_DOCS; doc = iter.nextDoc()) { | ||
| set(doc); | ||
| int nextPossibleNonMatchingDocID = iter.peekNextNonMatchingDocID(); |
There was a problem hiding this comment.
This API would no longer returns NO_MORE_DOCS when the underlying iterator has exhausted. Please see the discussion here apache#12194 (comment)
There was a problem hiding this comment.
Ah ok, from the the discussion then, it seems like I can just remove the if statement.
| } | ||
|
|
||
| for (int i = startIndex; i < endIndex; i++) { | ||
| set(i); |
There was a problem hiding this comment.
Hmmm I feel this might not be optimal actually. Could we utilize SparseFixedBitSet's specialized data structure to set a range of docIDs at once, similar to how FixedBitSet utilized its special data structure?
There was a problem hiding this comment.
Yeah, I figured there would be a better way, but this was just a quick override so I could run a perf test. I'll see if I can make this better.
| @@ -104,7 +107,19 @@ protected final void checkUnpositioned(DocIdSetIterator iter) { | |||
| public void or(DocIdSetIterator iter) throws IOException { | |||
There was a problem hiding this comment.
I'm also wondering if the implementation here will be utilized effectively, since it has been overridden in subclasses?
There was a problem hiding this comment.
I checked the overrides and there were only 2. The first was FixedBitSet which just calls super.or in the case of a non-bitset DocIdSetIterator. The other override is in SparseFixedBitSet which shouldn't be really benefitting from this optimization anyways (I'm assuming there won't be long runs of docs in sparse bitsets).
|
Thanks for taking a look at this! Looking at Adrien's comment in the main PR, it seems that this should be a separate PR made after your work is merged? I'll continue to work on this but maybe I should close this pull request then? |
I think as long as you keep dependence to my changes to the minimum (namely, probably just the new API |
|
Hey, sorry for the delayed response, but I was able to also test this optimization on some internal (Amazon Search) benchmarks and also saw no difference with or without this optimization. I think given that I saw no changes with this in the |
Description
Please provide a short description of the changes you're making with this pull request.
Solution
Please provide a short description of the approach taken to implement your solution.
Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.