Correct TermOrdValComparator competitive iterator intoBitSet implementation #14523
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-project-automation
bot
moved this to Open
in OpenSearch Lucene & Core Performance Tracking
…re similar to other impls
benwtrent
changed the title
benwtrent
commented
Apr 18, 2025
lucene/core/src/java/org/apache/lucene/search/DisjunctionDISIApproximation.java
Outdated
Show resolved
Hide resolved
lucene/core/src/java/org/apache/lucene/search/comparators/NumericComparator.java
Outdated
Show resolved
Hide resolved
lucene/core/src/java/org/apache/lucene/search/comparators/TermOrdValComparator.java
Show resolved
Hide resolved
lucene/core/src/java/org/apache/lucene/search/comparators/TermOrdValComparator.java
Show resolved
Hide resolved
lucene/core/src/java/org/apache/lucene/search/comparators/NumericComparator.java
Outdated
Show resolved
Hide resolved
lucene/core/src/java/org/apache/lucene/search/DenseConjunctionBulkScorer.java
Outdated
Show resolved
Hide resolved
gf2121
reviewed
Apr 18, 2025
lucene/core/src/java/org/apache/lucene/search/DenseConjunctionBulkScorer.java
Outdated
Show resolved
Hide resolved
lucene/core/src/java/org/apache/lucene/search/DisjunctionDISIApproximation.java
Outdated
Show resolved
Hide resolved
lucene/core/src/java/org/apache/lucene/search/DocIdSetIterator.java
Outdated
Show resolved
Hide resolved
benwtrent
commented
Apr 18, 2025
| @@ -434,6 +434,9 @@ public void intoBitSet(int upTo, FixedBitSet bitSet, int offset) throws IOExcept | |||
| if (competitiveIterator.docID() < doc) { | |||
There was a problem hiding this comment.
This part is strange. All other intoBitSet impls that I can find call the competitiveIterator to advance to at least offset, not to some internal doc. I suppose NumericComparator is unique?
benwtrent
commented
Apr 18, 2025
lucene/core/src/java/org/apache/lucene/search/DisjunctionDISIApproximation.java
Outdated
Show resolved
Hide resolved
gf2121
reviewed
Apr 19, 2025
lucene/core/src/java/org/apache/lucene/search/DisjunctionDISIApproximation.java
Outdated
Show resolved
Hide resolved
lucene/core/src/java/org/apache/lucene/search/comparators/TermOrdValComparator.java
Show resolved
Hide resolved
benwtrent
changed the title
gf2121
approved these changes
Apr 19, 2025
| LeafCollector leafCollector = collector.getLeafCollector(context); | ||
| BulkScorer bulkScorer = weight.bulkScorer(context); | ||
| // We need to split on this specific doc ID so that the current doc of the competitive | ||
| // iterator |
There was a problem hiding this comment.
Nit: maybe format this single-line word better?
github-project-automation
bot
moved this from Open
to Merged
in OpenSearch Lucene & Core Performance Tracking
benwtrent
added a commit
that referenced
this pull request
Apr 19, 2025
…tation (#14523) Similar to @ChrisHegarty 's change for the count fix, this will move up the max check to before the into bit set. It seems like we could be calling intobitset erroneously on some edge cases. Additionally, TermOrdValComparator's competitive iterator will ensure that the docsWithField iterator (used when not all docs have a given field), is at least the baseline of the scoring window when doing intoBitSet. closes: #14517
benwtrent
added a commit
that referenced
this pull request
Apr 19, 2025
…tation (#14523) Similar to @ChrisHegarty 's change for the count fix, this will move up the max check to before the into bit set. It seems like we could be calling intobitset erroneously on some edge cases. Additionally, TermOrdValComparator's competitive iterator will ensure that the docsWithField iterator (used when not all docs have a given field), is at least the baseline of the scoring window when doing intoBitSet. closes: #14517
jpountz
reviewed
Apr 20, 2025
| if (upTo <= doc) { | ||
| return; | ||
| } | ||
| // Optimize the case when intersecting the competitive iterator is expensive, which is when it | ||
| // hasn't nailed down a disjunction of competitive terms yet. | ||
| if (disjunction == null) { | ||
| if (docsWithField != null) { | ||
| // we need to be absolutely sure that the iterator is at least at offset | ||
| if (docsWithField.docID() < offset) { |
There was a problem hiding this comment.
The contract from DocIdSetIterator#intoBitSet requires starting from the current doc rather than the first doc on or after offset, so it would be more correct to advance to doc here. In practice, it wouldn't make a difference today since all call sites first advance iterators to offset before calling intoBitSet, but it may not always be the case.
There was a problem hiding this comment.
The contract from DocIdSetIterator#intoBitSet requires starting from the current doc rather than the first doc on or after offset, so it would be more correct to advance to doc here.
So things might got broken when current doc is after the first doc like
CompetitiveIterator competitiveIterator = CompetitiveIterator.of(1, 2, 3, 4);
competitiveIterator.advance(2); // advance current doc to 2
competitiveIterator.update(0, 2000); // update to doc value
final int offset = 1;
if (competitiveIterator.docID() < offset) {
competitiveIterator.advance(offset);
}
competitiveIterator.intoBitSet(upTo, bitset, offset); // we should start from 2 instead of 1.
? Nice catch!
This was referenced Apr 21, 2025
jpountz
pushed a commit
to jpountz/lucene
that referenced
this pull request
Apr 24, 2025
…tation (apache#14523) Similar to @ChrisHegarty 's change for the count fix, this will move up the max check to before the into bit set. It seems like we could be calling intobitset erroneously on some edge cases. Additionally, TermOrdValComparator's competitive iterator will ensure that the docsWithField iterator (used when not all docs have a given field), is at least the baseline of the scoring window when doing intoBitSet. closes: apache#14517
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Similar to @ChrisHegarty 's change for the count fix, this will move up the
maxcheck to before the into bit set. It seems like we could be calling intobitset erroneously on some edge cases.Additionally, TermOrdValComparator's competitive iterator will ensure that the docsWithField iterator (used when not all docs have a given field), is at least the baseline of the scoring window when doing intoBitSet.
closes: #14517