romseygeek commented on code in PR #16450:
URL: https://github.com/apache/lucene/pull/16450#discussion_r3703301882
##########
lucene/core/src/java/org/apache/lucene/search/DocValuesRangeIterator.java:
##########
@@ -320,16 +333,22 @@ private BulkBlockRangeIterator(
@Override
public final boolean matches() throws IOException {
- return switch (blockIterator.getMatch()) {
- case YES -> true;
- case YES_IF_PRESENT -> advanceDisi(blockIterator.docID());
- case MAYBE -> advanceDisi(blockIterator.docID()) && predicate.get();
- };
+ return recordMatch(
+ switch (blockIterator.getMatch()) {
+ case YES -> true;
+ case YES_IF_PRESENT -> advanceDisi(blockIterator.docID());
+ case MAYBE -> advanceDisi(blockIterator.docID()) &&
predicate.get();
+ });
}
@Override
public final int docIDRunEnd() throws IOException {
- return blockIterator.docIDRunEnd();
+ // docIDRunEnd() may be called on non-matches, so only YES proves that
the current doc and the
+ // rest of the run are actual matches.
+ return switch (blockIterator.getMatch()) {
+ case YES -> blockIterator.docIDRunEnd();
+ case YES_IF_PRESENT, MAYBE -> confirmedDocRunEnd();
Review Comment:
Let's return `doc` for YES_IF_PRESENT and MAYBE
##########
lucene/core/src/java/org/apache/lucene/search/DocValuesRangeIterator.java:
##########
@@ -286,14 +287,26 @@ final boolean advanceDisi(int target) throws IOException {
return disi.advance(target) == target;
}
+ final boolean recordMatch(boolean matches) {
+ lastMatchingDoc = matches ? blockIterator.docID() : -1;
+ return matches;
+ }
+
+ final int confirmedDocRunEnd() {
+ int doc = blockIterator.docID();
+ return lastMatchingDoc == doc ? doc + 1 : doc;
+ }
+
@Override
public boolean matches() throws IOException {
- return advanceDisi(blockIterator.docID()) && predicate.get();
+ return recordMatch(advanceDisi(blockIterator.docID()) &&
predicate.get());
}
@Override
public int docIDRunEnd() throws IOException {
- return blockIterator.docID() + 1;
+ // Even a YES block only proves membership in the ordinal set's bounding
range, not in the
+ // potentially non-contiguous set itself.
+ return confirmedDocRunEnd();
Review Comment:
I think for the general case we can just fall back to the superclass
implementation here, ie don't bother subclassing at all.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
