Correct TermOrdValComparator competitive iterator intoBitSet implementation by benwtrent · Pull Request #14523 · apache/lucene

benwtrent · 2025-04-18T14:19:46Z

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

…erm-ord-comp

…re similar to other impls

lucene/core/src/java/org/apache/lucene/search/DisjunctionDISIApproximation.java

lucene/core/src/java/org/apache/lucene/search/comparators/NumericComparator.java

lucene/core/src/java/org/apache/lucene/search/comparators/TermOrdValComparator.java

lucene/core/src/java/org/apache/lucene/search/comparators/NumericComparator.java

lucene/core/src/java/org/apache/lucene/search/DenseConjunctionBulkScorer.java

ChrisHegarty

LGTM

lucene/core/src/java/org/apache/lucene/search/DenseConjunctionBulkScorer.java

lucene/core/src/java/org/apache/lucene/search/DisjunctionDISIApproximation.java

lucene/core/src/java/org/apache/lucene/search/DocIdSetIterator.java

…erm-ord-comp

benwtrent · 2025-04-18T17:53:15Z

lucene/core/src/java/org/apache/lucene/search/comparators/NumericComparator.java

@@ -434,6 +434,9 @@ public void intoBitSet(int upTo, FixedBitSet bitSet, int offset) throws IOExcept
          if (competitiveIterator.docID() < doc) {


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?

lucene/core/src/java/org/apache/lucene/search/DisjunctionDISIApproximation.java

lucene/core/src/java/org/apache/lucene/search/comparators/TermOrdValComparator.java

gf2121

Thank you

gf2121 · 2025-04-19T13:25:16Z

lucene/core/src/test/org/apache/lucene/search/comparators/TestTermOrdValComparator.java

+        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


Nit: maybe format this single-line word better?

@ChrisHegarty

…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

@ChrisHegarty

…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 · 2025-04-20T07:40:50Z

lucene/core/src/java/org/apache/lucene/search/comparators/TermOrdValComparator.java

      if (disjunction == null) {
        if (docsWithField != null) {
+          // we need to be absolutely sure that the iterator is at least at offset
+          if (docsWithField.docID() < offset) {


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.

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!

Yes, exactly.

@ChrisHegarty

…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

benwtrent added 3 commits April 3, 2025 07:52

Disable HNSW connectedComponents (apache#14214)

b50f8f7

Addressing potential past iteration of intoBitSet

d85eb89

Merge remote-tracking branch 'upstream/main' into bugfix/intobitset-t…

a7145ae

…erm-ord-comp

benwtrent added this to the 10.2.1 milestone Apr 18, 2025

github-project-automation bot added this to OpenSearch Lucene & Core Performance Tracking Apr 18, 2025

github-project-automation bot moved this to Open in OpenSearch Lucene & Core Performance Tracking Apr 18, 2025

benwtrent requested a review from ChrisHegarty April 18, 2025 14:19

github-actions bot added the module:core/search label Apr 18, 2025

address two other strange places for intoBitSet, adding checks that a…

e1bd32f

…re similar to other impls

benwtrent changed the title ~~intobitset term ord comp~~ Shore up some intoBitSet impls and add paranoid protections Apr 18, 2025

benwtrent commented Apr 18, 2025

View reviewed changes

formatting fixes

f692b63

ChrisHegarty reviewed Apr 18, 2025

View reviewed changes

ChrisHegarty approved these changes Apr 18, 2025

View reviewed changes

gf2121 reviewed Apr 18, 2025

View reviewed changes

benwtrent added 2 commits April 18, 2025 13:41

reverting some changes

c7d674f

Merge remote-tracking branch 'upstream/main' into bugfix/intobitset-t…

1519540

…erm-ord-comp

benwtrent commented Apr 18, 2025

View reviewed changes

always iterate docsWithField

8102d89

benwtrent commented Apr 18, 2025

View reviewed changes

lucene/core/src/java/org/apache/lucene/search/DisjunctionDISIApproximation.java Outdated Show resolved Hide resolved

gf2121 reviewed Apr 19, 2025

View reviewed changes

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 ~~Shore up some intoBitSet impls and add paranoid protections~~ Correct TermOrdValComparator competitive iterator intoBitSet implementation Apr 19, 2025

removing unnecessary changes, adding test

f6ce8df

benwtrent requested review from ChrisHegarty and gf2121 April 19, 2025 13:17

gf2121 approved these changes Apr 19, 2025

View reviewed changes

formatting

c39e7ae

benwtrent merged commit ca6879d into apache:main Apr 19, 2025
7 checks passed

github-project-automation bot moved this from Open to Merged in OpenSearch Lucene & Core Performance Tracking Apr 19, 2025

benwtrent deleted the bugfix/intobitset-term-ord-comp branch April 19, 2025 16:08

jpountz reviewed Apr 20, 2025

View reviewed changes

ChrisHegarty mentioned this pull request Apr 20, 2025

ArrayIndexOutOfBoundsException in FixedBitSet.set during query elastic/elasticsearch#126939

Closed

gf2121 mentioned this pull request Apr 21, 2025

Advance doc instead of offset in competitive iterator #14530

Merged

This was referenced Apr 21, 2025

Update Elasticsearch main with snapshot version of Lucene elastic/elasticsearch#127125

Merged

Update Elasticsearch main with snapshot version of Lucene (#127125) elastic/elasticsearch#127132

Merged

		@@ -434,6 +434,9 @@ public void intoBitSet(int upTo, FixedBitSet bitSet, int offset) throws IOExcept
		if (competitiveIterator.docID() < doc) {

Conversation

benwtrent commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChrisHegarty left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benwtrent Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gf2121 left a comment

Choose a reason for hiding this comment

Uh oh!

gf2121 Apr 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jpountz Apr 20, 2025

Choose a reason for hiding this comment

Uh oh!

gf2121 Apr 20, 2025

Choose a reason for hiding this comment

Uh oh!

jpountz Apr 20, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benwtrent commented Apr 18, 2025 •

edited

Loading