Skip to content

Throw exceptions if nextOrd called more than docValueCount times #17649

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
msfroh wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Throw exceptions if nextOrd called more than docValueCount times #17649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aec63cc
[DRAFT] Throw exceptions if nextOrd called more than docValueCount times
msfroh Mar 21, 2025
85a97b3
Fix docValueCount in MultiOrdinals$MultiDocs
msfroh Mar 31, 2025
7a4eac3
Fix some invalid calls to nextOrd
msfroh Mar 31, 2025
e2d6961
Simplify loop in GlobalOrdinalsStringTermsAggregator
msfroh Mar 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions server/src/main/java/org/opensearch/index/fielddata/ordinals/GlobalOrdinalMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,11 @@
@Override
public long nextOrd() throws IOException {
if (++nextOrd > docValueCount) {
return SortedSetDocValues.NO_MORE_DOCS;
throw new IllegalStateException("Called nextOrd after docValueCount");

Check warning on line 92 in server/src/main/java/org/opensearch/index/fielddata/ordinals/GlobalOrdinalMapping.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/index/fielddata/ordinals/GlobalOrdinalMapping.java#L92 

Added line #L92 was not covered by tests
}
long segmentOrd = values.nextOrd();
if (segmentOrd == SortedSetDocValues.NO_MORE_DOCS) {
return SortedSetDocValues.NO_MORE_DOCS;
throw new IllegalStateException("Wrapped values returned no more docs too early");

Check warning on line 96 in server/src/main/java/org/opensearch/index/fielddata/ordinals/GlobalOrdinalMapping.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/index/fielddata/ordinals/GlobalOrdinalMapping.java#L96 

Added line #L96 was not covered by tests
} else {
return getGlobalOrd(segmentOrd);
}
Expand Down
8 changes: 5 additions & 3 deletions server/src/main/java/org/opensearch/index/fielddata/ordinals/MultiOrdinals.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@

private long currentOffset;
private long currentEndOffset;
private int docValueCount;

MultiDocs(MultiOrdinals ordinals, ValuesHolder values) {
this.valueCount = ordinals.valueCount;
Expand All @@ -213,13 +214,14 @@
public boolean advanceExact(int docId) throws IOException {
currentOffset = docId != 0 ? endOffsets.get(docId - 1) : 0;
currentEndOffset = endOffsets.get(docId);
return currentOffset != currentEndOffset;
docValueCount = Math.toIntExact(currentEndOffset - currentOffset);
return docValueCount > 0;
}

@Override
public long nextOrd() throws IOException {
if (currentOffset == currentEndOffset) {
return SortedSetDocValues.NO_MORE_DOCS;
throw new IllegalStateException("nextOrd() called more than docValueCount() times");

Check warning on line 224 in server/src/main/java/org/opensearch/index/fielddata/ordinals/MultiOrdinals.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/index/fielddata/ordinals/MultiOrdinals.java#L224 

Added line #L224 was not covered by tests
Copy link
Contributor

@jainankitk jainankitk Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if this exception and Called nextOrd after docValueCount have different meaning? If not, we should reuse the same message for all instances? Should we create custom exception for this case?

Copy link
Contributor Author

@msfroh msfroh Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Called nextOrd after docValueCount was something generated by IntelliJ's auto-complete. This message is better IMO. I can make it consistent across the different implementations.

In practice, these checks should be short-lived. Lucene doesn't even do checks. The JavaDoc there just says "It is illegal to call this more than docValueCount() times for the currently-positioned doc." Some implementations will just return the same value over and over. Other implementations may do something else.

I added these exceptions to highlight the places where we were doing the wrong thing. The later commits on this PR were where I fixed those places. In theory, before we ship OpenSearch 3.0, we could probably remove these exceptions.

Copy link
Contributor

@jainankitk jainankitk Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, let us make these exceptions consistent.

} else {
return ords.get(currentOffset++);
}
Expand All @@ -232,7 +234,7 @@

@Override
public int docValueCount() {
return Math.toIntExact(currentEndOffset - currentOffset);
return docValueCount;
}
}
}
6 changes: 2 additions & 4 deletions .../org/opensearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,11 +556,9 @@ public void collect(int doc, long owningBucketOrd) throws IOException {
if (false == segmentOrds.advanceExact(doc)) {
return;
}
int count = segmentOrds.docValueCount();
long segmentOrd;
while ((count-- > 0) && (segmentOrd = segmentOrds.nextOrd()) != SortedSetDocValues.NO_MORE_DOCS) {
for (int i = 0; i < segmentOrds.docValueCount(); i++) {
long docCount = docCountProvider.getDocCount(doc);
segmentDocCounts.increment(segmentOrd + 1, docCount);
segmentDocCounts.increment(segmentOrds.nextOrd() + 1, docCount);
}
}
});
Expand Down
8 changes: 8 additions & 0 deletions server/src/main/java/org/opensearch/search/aggregations/support/MissingValues.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@
static SortedSetDocValues insertOrd(final SortedSetDocValues values, final long insertedOrd, final BytesRef missingValue) {
return new AbstractSortedSetDocValues() {

private int ordCount = 0;
private boolean hasOrds;
private long nextMissingOrd;

Expand Down Expand Up @@ -367,6 +368,9 @@
@Override
public long nextOrd() throws IOException {
if (hasOrds) {
if (++ordCount > values.docValueCount()) {
Copy link
Contributor

@expani expani Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@msfroh This is failing due to the check we are doing here.

The values here is MultiOrdinals$MultiOrds ( for TermsAggregatorTests #testBucketInTermsAggregationWithMissingValue ) and its docValueCount decreases with each call to nextOrd

 public int docValueCount() {
      return Math.toIntExact(currentEndOffset - currentOffset);
}

Copy link
Contributor

@expani expani Mar 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior is the same since Lucene 9.2.0 upgrade so shouldn't be a problem. WDYT ?

Copy link
Contributor Author

@msfroh msfroh Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... that implementation in MultiDocs seems wrong. The response for docValueCount() should only change when we move to another doc. If it's changing as a result of nextOrd calls, that's wrong.

throw new IllegalStateException("Called nextOrd after docValueCount");

Check warning on line 372 in server/src/main/java/org/opensearch/search/aggregations/support/MissingValues.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/search/aggregations/support/MissingValues.java#L372 

Added line #L372 was not covered by tests
}
final long ord = values.nextOrd();
if (ord < insertedOrd) {
return ord;
Expand All @@ -376,6 +380,9 @@
return ord + 1;
}
} else {
if (nextMissingOrd == SortedSetDocValues.NO_MORE_DOCS) {
throw new IllegalStateException("Called nextOrd after docValueCount");

Check warning on line 384 in server/src/main/java/org/opensearch/search/aggregations/support/MissingValues.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/search/aggregations/support/MissingValues.java#L384 

Added line #L384 was not covered by tests
}
// we want to return the next missing ord but set this to
// NO_MORE_DOCS so on the next call we indicate there are no
// more values
Expand All @@ -388,6 +395,7 @@
@Override
public boolean advanceExact(int doc) throws IOException {
hasOrds = values.advanceExact(doc);
ordCount = 0;
nextMissingOrd = insertedOrd;
// always return true because we want to return a value even if
// the document does not have a value
Expand Down
19 changes: 6 additions & 13 deletions server/src/test/java/org/opensearch/index/fielddata/AbstractStringFieldDataTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
import org.apache.lucene.index.Term;
import org.apache.lucene.index.TermsEnum;
import org.apache.lucene.search.ConstantScoreQuery;
import org.apache.lucene.search.DocIdSetIterator;
import org.apache.lucene.search.IndexSearcher;
import org.apache.lucene.search.MatchAllDocsQuery;
import org.apache.lucene.search.Query;
Expand Down Expand Up @@ -499,27 +498,26 @@ public void testGlobalOrdinals() throws Exception {
LeafOrdinalsFieldData afd = globalOrdinals.load(leaf);
SortedSetDocValues values = afd.getOrdinalsValues();
assertTrue(values.advanceExact(0));
assertEquals(2, values.docValueCount());
long ord = values.nextOrd();
assertThat(ord, equalTo(3L));
assertThat(values.lookupOrd(ord).utf8ToString(), equalTo("02"));
ord = values.nextOrd();
assertThat(ord, equalTo(5L));
assertThat(values.lookupOrd(ord).utf8ToString(), equalTo("04"));
ord = values.nextOrd();
assertThat(ord, equalTo((long) DocIdSetIterator.NO_MORE_DOCS));
assertFalse(values.advanceExact(1));
assertTrue(values.advanceExact(2));
assertEquals(1, values.docValueCount());
ord = values.nextOrd();
assertThat(ord, equalTo(4L));
assertThat(values.lookupOrd(ord).utf8ToString(), equalTo("03"));
ord = values.nextOrd();
assertThat(ord, equalTo((long) DocIdSetIterator.NO_MORE_DOCS));

// Second segment
leaf = topLevelReader.leaves().get(1);
afd = globalOrdinals.load(leaf);
values = afd.getOrdinalsValues();
assertTrue(values.advanceExact(0));
assertEquals(3, values.docValueCount());
ord = values.nextOrd();
assertThat(ord, equalTo(5L));
assertThat(values.lookupOrd(ord).utf8ToString(), equalTo("04"));
Expand All @@ -529,9 +527,8 @@ public void testGlobalOrdinals() throws Exception {
ord = values.nextOrd();
assertThat(ord, equalTo(7L));
assertThat(values.lookupOrd(ord).utf8ToString(), equalTo("06"));
ord = values.nextOrd();
assertThat(ord, equalTo((long) DocIdSetIterator.NO_MORE_DOCS));
assertTrue(values.advanceExact(1));
assertEquals(3, values.docValueCount());
ord = values.nextOrd();
assertThat(ord, equalTo(7L));
assertThat(values.lookupOrd(ord).utf8ToString(), equalTo("06"));
Expand All @@ -541,10 +538,9 @@ public void testGlobalOrdinals() throws Exception {
ord = values.nextOrd();
assertThat(ord, equalTo(9L));
assertThat(values.lookupOrd(ord).utf8ToString(), equalTo("08"));
ord = values.nextOrd();
assertThat(ord, equalTo((long) DocIdSetIterator.NO_MORE_DOCS));
assertFalse(values.advanceExact(2));
assertTrue(values.advanceExact(3));
assertEquals(3, values.docValueCount());
ord = values.nextOrd();
assertThat(ord, equalTo(9L));
assertThat(values.lookupOrd(ord).utf8ToString(), equalTo("08"));
Expand All @@ -554,14 +550,13 @@ public void testGlobalOrdinals() throws Exception {
ord = values.nextOrd();
assertThat(ord, equalTo(11L));
assertThat(values.lookupOrd(ord).utf8ToString(), equalTo("10"));
ord = values.nextOrd();
assertThat(ord, equalTo((long) DocIdSetIterator.NO_MORE_DOCS));

// Third segment
leaf = topLevelReader.leaves().get(2);
afd = globalOrdinals.load(leaf);
values = afd.getOrdinalsValues();
assertTrue(values.advanceExact(0));
assertEquals(3, values.docValueCount());
ord = values.nextOrd();
assertThat(ord, equalTo(0L));
assertThat(values.lookupOrd(ord).utf8ToString(), equalTo("!08"));
Expand All @@ -571,8 +566,6 @@ public void testGlobalOrdinals() throws Exception {
ord = values.nextOrd();
assertThat(ord, equalTo(2L));
assertThat(values.lookupOrd(ord).utf8ToString(), equalTo("!10"));
ord = values.nextOrd();
assertThat(ord, equalTo((long) DocIdSetIterator.NO_MORE_DOCS));
}

public void testTermsEnum() throws Exception {
Expand Down
Loading