Skip to content

Commit

Permalink
Fix GlobalOrdinalMapping and GlobalOrdinalsStringTermsAggregator ordi…
Browse files Browse the repository at this point in the history 
…nal traversals

Signed-off-by: Andriy Redko <[email protected]>
  • Loading branch information
@reta
reta committed Dec 4, 2024
1 parent 61ee1b4 commit d44b0d8
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 17 deletions.
15 changes: 13 additions & 2 deletions server/src/main/java/org/opensearch/index/fielddata/ordinals/GlobalOrdinalMapping.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ final class GlobalOrdinalMapping extends SortedSetDocValues {
private final OrdinalMap ordinalMap;
private final LongValues mapping;
private final TermsEnum[] lookups;
private int docValueCount = 0;
private int nextOrd = 0;

GlobalOrdinalMapping(OrdinalMap ordinalMap, SortedSetDocValues values, TermsEnum[] lookups, int segmentIndex) {
Expand All @@ -72,12 +73,22 @@ public long getGlobalOrd(long segmentOrd) {

@Override
public boolean advanceExact(int target) throws IOException {
return values.advanceExact(target);
nextOrd = 0; /* reset next ordinal */
docValueCount = 0; /* reset docValueCount */
if (values.advanceExact(target)) {
// Some SortedSetDocValues implementations like MultiOrdinals#MultiDocs do change
// docValueCount() return value after each nextOrd() call, so we prefetch the value
// here.
docValueCount = values.docValueCount();
return true;
} else {
return false;
}
}

@Override
public long nextOrd() throws IOException {
if (++nextOrd > values.docValueCount()) {
if (++nextOrd > docValueCount) {
return SortedSetDocValues.NO_MORE_DOCS;
}
long segmentOrd = values.nextOrd();
Expand Down
18 changes: 9 additions & 9 deletions .../org/opensearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -288,9 +288,9 @@ public void collect(int doc, long owningBucketOrd) throws IOException {
if (false == globalOrds.advanceExact(doc)) {
return;
}
int count = 0;
for (long globalOrd = globalOrds.nextOrd(); globalOrd != NO_MORE_DOCS && count < globalOrds.docValueCount(); globalOrd =
globalOrds.nextOrd(), ++count) {
int count = globalOrds.docValueCount();
long globalOrd;
while ((count-- > 0) && (globalOrd = globalOrds.nextOrd()) != SortedSetDocValues.NO_MORE_DOCS) {
collectionStrategy.collectGlobalOrd(owningBucketOrd, doc, globalOrd, sub);
}
}
Expand All @@ -302,9 +302,9 @@ public void collect(int doc, long owningBucketOrd) throws IOException {
if (false == globalOrds.advanceExact(doc)) {
return;
}
int count = 0;
for (long globalOrd = globalOrds.nextOrd(); globalOrd != NO_MORE_DOCS && count < globalOrds.docValueCount(); globalOrd =
globalOrds.nextOrd(), ++count) {
int count = globalOrds.docValueCount();
long globalOrd;
while ((count-- > 0) && (globalOrd = globalOrds.nextOrd()) != SortedSetDocValues.NO_MORE_DOCS) {
if (false == acceptedGlobalOrdinals.test(globalOrd)) {
continue;
}
Expand Down Expand Up @@ -481,9 +481,9 @@ public void collect(int doc, long owningBucketOrd) throws IOException {
if (false == segmentOrds.advanceExact(doc)) {
return;
}
int count = 0;
for (long segmentOrd = segmentOrds.nextOrd(); segmentOrd != NO_MORE_DOCS
&& count < segmentOrds.docValueCount(); segmentOrd = segmentOrds.nextOrd(), ++count) {
int count = segmentOrds.docValueCount();
long segmentOrd;
while ((count-- > 0) && (segmentOrd = segmentOrds.nextOrd()) != SortedSetDocValues.NO_MORE_DOCS) {
long docCount = docCountProvider.getDocCount(doc);
segmentDocCounts.increment(segmentOrd + 1, docCount);
}
Expand Down
12 changes: 6 additions & 6 deletions server/src/test/java/org/opensearch/index/fielddata/AbstractStringFieldDataTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -506,14 +506,14 @@ public void testGlobalOrdinals() throws Exception {
assertThat(ord, equalTo(5L));
assertThat(values.lookupOrd(ord).utf8ToString(), equalTo("04"));
ord = values.nextOrd();
assertThat(ord, equalTo(DocIdSetIterator.NO_MORE_DOCS));
assertThat(ord, equalTo((long) DocIdSetIterator.NO_MORE_DOCS));
assertFalse(values.advanceExact(1));
assertTrue(values.advanceExact(2));
ord = values.nextOrd();
assertThat(ord, equalTo(4L));
assertThat(values.lookupOrd(ord).utf8ToString(), equalTo("03"));
ord = values.nextOrd();
assertThat(ord, equalTo(DocIdSetIterator.NO_MORE_DOCS));
assertThat(ord, equalTo((long) DocIdSetIterator.NO_MORE_DOCS));

// Second segment
leaf = topLevelReader.leaves().get(1);
Expand All @@ -530,7 +530,7 @@ public void testGlobalOrdinals() throws Exception {
assertThat(ord, equalTo(7L));
assertThat(values.lookupOrd(ord).utf8ToString(), equalTo("06"));
ord = values.nextOrd();
assertThat(ord, equalTo(DocIdSetIterator.NO_MORE_DOCS));
assertThat(ord, equalTo((long) DocIdSetIterator.NO_MORE_DOCS));
assertTrue(values.advanceExact(1));
ord = values.nextOrd();
assertThat(ord, equalTo(7L));
Expand All @@ -542,7 +542,7 @@ public void testGlobalOrdinals() throws Exception {
assertThat(ord, equalTo(9L));
assertThat(values.lookupOrd(ord).utf8ToString(), equalTo("08"));
ord = values.nextOrd();
assertThat(ord, equalTo(DocIdSetIterator.NO_MORE_DOCS));
assertThat(ord, equalTo((long) DocIdSetIterator.NO_MORE_DOCS));
assertFalse(values.advanceExact(2));
assertTrue(values.advanceExact(3));
ord = values.nextOrd();
Expand All @@ -555,7 +555,7 @@ public void testGlobalOrdinals() throws Exception {
assertThat(ord, equalTo(11L));
assertThat(values.lookupOrd(ord).utf8ToString(), equalTo("10"));
ord = values.nextOrd();
assertThat(ord, equalTo(DocIdSetIterator.NO_MORE_DOCS));
assertThat(ord, equalTo((long) DocIdSetIterator.NO_MORE_DOCS));

// Third segment
leaf = topLevelReader.leaves().get(2);
Expand All @@ -572,7 +572,7 @@ public void testGlobalOrdinals() throws Exception {
assertThat(ord, equalTo(2L));
assertThat(values.lookupOrd(ord).utf8ToString(), equalTo("!10"));
ord = values.nextOrd();
assertThat(ord, equalTo(DocIdSetIterator.NO_MORE_DOCS));
assertThat(ord, equalTo((long) DocIdSetIterator.NO_MORE_DOCS));
}

public void testTermsEnum() throws Exception {
Expand Down

0 comments on commit d44b0d8

Please sign in to comment.