diff --git a/server/src/main/java/org/opensearch/index/fielddata/ordinals/GlobalOrdinalMapping.java b/server/src/main/java/org/opensearch/index/fielddata/ordinals/GlobalOrdinalMapping.java index 8f2bd0f5f5cab..e4fcd0958342d 100644 --- a/server/src/main/java/org/opensearch/index/fielddata/ordinals/GlobalOrdinalMapping.java +++ b/server/src/main/java/org/opensearch/index/fielddata/ordinals/GlobalOrdinalMapping.java @@ -89,11 +89,11 @@ public boolean advanceExact(int target) throws IOException { @Override public long nextOrd() throws IOException { if (++nextOrd > docValueCount) { - return SortedSetDocValues.NO_MORE_DOCS; + throw new IllegalStateException("Called nextOrd after docValueCount"); } 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"); } else { return getGlobalOrd(segmentOrd); } diff --git a/server/src/main/java/org/opensearch/index/fielddata/ordinals/MultiOrdinals.java b/server/src/main/java/org/opensearch/index/fielddata/ordinals/MultiOrdinals.java index ea91e6ddd1820..69a95a4795028 100644 --- a/server/src/main/java/org/opensearch/index/fielddata/ordinals/MultiOrdinals.java +++ b/server/src/main/java/org/opensearch/index/fielddata/ordinals/MultiOrdinals.java @@ -196,6 +196,7 @@ private static class MultiDocs extends AbstractSortedSetDocValues { private long currentOffset; private long currentEndOffset; + private int docValueCount; MultiDocs(MultiOrdinals ordinals, ValuesHolder values) { this.valueCount = ordinals.valueCount; @@ -213,13 +214,14 @@ public long getValueCount() { 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"); } else { return ords.get(currentOffset++); } @@ -232,7 +234,7 @@ public BytesRef lookupOrd(long ord) { @Override public int docValueCount() { - return Math.toIntExact(currentEndOffset - currentOffset); + return docValueCount; } } } diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java index d8ec9feaf44b4..d9e534d9b4552 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java @@ -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); } } }); diff --git a/server/src/main/java/org/opensearch/search/aggregations/support/MissingValues.java b/server/src/main/java/org/opensearch/search/aggregations/support/MissingValues.java index 166334292d438..0e9fcb9cd4a54 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/support/MissingValues.java +++ b/server/src/main/java/org/opensearch/search/aggregations/support/MissingValues.java @@ -339,6 +339,7 @@ public String toString() { 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; @@ -367,6 +368,9 @@ public int docValueCount() { @Override public long nextOrd() throws IOException { if (hasOrds) { + if (++ordCount > values.docValueCount()) { + throw new IllegalStateException("Called nextOrd after docValueCount"); + } final long ord = values.nextOrd(); if (ord < insertedOrd) { return ord; @@ -376,6 +380,9 @@ public long nextOrd() throws IOException { return ord + 1; } } else { + if (nextMissingOrd == SortedSetDocValues.NO_MORE_DOCS) { + throw new IllegalStateException("Called nextOrd after docValueCount"); + } // 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 @@ -388,6 +395,7 @@ public long nextOrd() throws IOException { @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 diff --git a/server/src/test/java/org/opensearch/index/fielddata/AbstractStringFieldDataTestCase.java b/server/src/test/java/org/opensearch/index/fielddata/AbstractStringFieldDataTestCase.java index da314358475c4..a242974a3a7dc 100644 --- a/server/src/test/java/org/opensearch/index/fielddata/AbstractStringFieldDataTestCase.java +++ b/server/src/test/java/org/opensearch/index/fielddata/AbstractStringFieldDataTestCase.java @@ -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; @@ -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")); @@ -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")); @@ -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")); @@ -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")); @@ -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 {