From d44b0d8d4a29d5418666379495246a9487f71628 Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Wed, 4 Dec 2024 11:55:36 -0500 Subject: [PATCH] Fix GlobalOrdinalMapping and GlobalOrdinalsStringTermsAggregator ordinal traversals Signed-off-by: Andriy Redko --- .../ordinals/GlobalOrdinalMapping.java | 15 +++++++++++++-- .../GlobalOrdinalsStringTermsAggregator.java | 18 +++++++++--------- .../AbstractStringFieldDataTestCase.java | 12 ++++++------ 3 files changed, 28 insertions(+), 17 deletions(-) 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 1d5b111102d0e..8f2bd0f5f5cab 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 @@ -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) { @@ -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(); 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 96554920084b6..9dbc97f7d2cb6 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 @@ -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); } } @@ -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; } @@ -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); } 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 54f882c22031a..da314358475c4 100644 --- a/server/src/test/java/org/opensearch/index/fielddata/AbstractStringFieldDataTestCase.java +++ b/server/src/test/java/org/opensearch/index/fielddata/AbstractStringFieldDataTestCase.java @@ -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); @@ -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)); @@ -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(); @@ -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); @@ -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 {