You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by gs...@apache.org on 2023/10/13 14:24:47 UTC
[lucene] branch main updated: Ensure LeafCollector#finish is only called once on the main collector during drill-sideways (#12642)
This is an automated email from the ASF dual-hosted git repository.
gsmiller pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/lucene.git
The following commit(s) were added to refs/heads/main by this push:
new 7b7b0d21195 Ensure LeafCollector#finish is only called once on the main collector during drill-sideways (#12642)
7b7b0d21195 is described below
commit 7b7b0d21195916e79f6e6b1eef52c7734f76f5d6
Author: Greg Miller <gs...@gmail.com>
AuthorDate: Fri Oct 13 07:24:40 2023 -0700
Ensure LeafCollector#finish is only called once on the main collector during drill-sideways (#12642)
---
lucene/CHANGES.txt | 2 +
.../apache/lucene/facet/DrillSidewaysScorer.java | 14 +++---
.../org/apache/lucene/facet/TestDrillSideways.java | 54 +++++++++++++++++++++-
.../lucene/tests/search/AssertingCollector.java | 15 ++++--
4 files changed, 75 insertions(+), 10 deletions(-)
diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt
index 5ed0623940b..5a67abc0be7 100644
--- a/lucene/CHANGES.txt
+++ b/lucene/CHANGES.txt
@@ -200,6 +200,8 @@ Bug Fixes
* GITHUB#11556: HTMLStripCharFilter fails on '>' or '<' characters in attribute values. (Elliot Lin)
+* GITHUB#12642: Ensure #finish only gets called once on the base collector during drill-sideways (Greg Miller)
+
Build
---------------------
diff --git a/lucene/facet/src/java/org/apache/lucene/facet/DrillSidewaysScorer.java b/lucene/facet/src/java/org/apache/lucene/facet/DrillSidewaysScorer.java
index 27118eaa59e..cddc0ef69bc 100644
--- a/lucene/facet/src/java/org/apache/lucene/facet/DrillSidewaysScorer.java
+++ b/lucene/facet/src/java/org/apache/lucene/facet/DrillSidewaysScorer.java
@@ -199,7 +199,7 @@ class DrillSidewaysScorer extends BulkScorer {
docID = baseApproximation.nextDoc();
}
- finish(collector, Collections.singleton(dim));
+ finish(Collections.singleton(dim));
}
/**
@@ -338,7 +338,7 @@ class DrillSidewaysScorer extends BulkScorer {
docID = baseApproximation.nextDoc();
}
- finish(collector, sidewaysDims);
+ finish(sidewaysDims);
}
private static int advanceIfBehind(int docID, DocIdSetIterator iterator) throws IOException {
@@ -557,7 +557,7 @@ class DrillSidewaysScorer extends BulkScorer {
nextChunkStart += CHUNK;
}
- finish(collector, Arrays.asList(dims));
+ finish(Arrays.asList(dims));
}
private void doUnionScoring(Bits acceptDocs, LeafCollector collector, DocsAndCost[] dims)
@@ -713,7 +713,7 @@ class DrillSidewaysScorer extends BulkScorer {
nextChunkStart += CHUNK;
}
- finish(collector, Arrays.asList(dims));
+ finish(Arrays.asList(dims));
}
private void collectHit(LeafCollector collector, DocsAndCost[] dims) throws IOException {
@@ -765,8 +765,10 @@ class DrillSidewaysScorer extends BulkScorer {
sidewaysCollector.collect(collectDocID);
}
- private void finish(LeafCollector collector, Collection<DocsAndCost> dims) throws IOException {
- collector.finish();
+ private void finish(Collection<DocsAndCost> dims) throws IOException {
+ // Note: We _only_ call #finish on the facets collectors we're managing here, but not the
+ // "main" collector. This is because IndexSearcher handles calling #finish on the main
+ // collector.
if (drillDownLeafCollector != null) {
drillDownLeafCollector.finish();
}
diff --git a/lucene/facet/src/test/org/apache/lucene/facet/TestDrillSideways.java b/lucene/facet/src/test/org/apache/lucene/facet/TestDrillSideways.java
index 2f322ff73c8..1bc40fea5c7 100644
--- a/lucene/facet/src/test/org/apache/lucene/facet/TestDrillSideways.java
+++ b/lucene/facet/src/test/org/apache/lucene/facet/TestDrillSideways.java
@@ -81,6 +81,7 @@ import org.apache.lucene.search.Weight;
import org.apache.lucene.store.Directory;
import org.apache.lucene.tests.analysis.MockAnalyzer;
import org.apache.lucene.tests.index.RandomIndexWriter;
+import org.apache.lucene.tests.search.AssertingCollector;
import org.apache.lucene.tests.util.TestUtil;
import org.apache.lucene.util.BytesRef;
import org.apache.lucene.util.IOUtils;
@@ -316,6 +317,58 @@ public class TestDrillSideways extends FacetTestCase {
IOUtils.close(searcher.getIndexReader(), taxoReader, taxoWriter, dir, taxoDir);
}
+ public void testLeafCollectorSingleFinishCall() throws Exception {
+ try (Directory dir = newDirectory();
+ Directory taxoDir = newDirectory();
+ RandomIndexWriter w = new RandomIndexWriter(random(), dir);
+ DirectoryTaxonomyWriter taxoW =
+ new DirectoryTaxonomyWriter(taxoDir, IndexWriterConfig.OpenMode.CREATE)) {
+ FacetsConfig facetsConfig = new FacetsConfig();
+
+ Document d = new Document();
+ d.add(new FacetField("foo", "bar"));
+ w.addDocument(facetsConfig.build(taxoW, d));
+
+ try (IndexReader r = w.getReader();
+ TaxonomyReader taxoR = new DirectoryTaxonomyReader(taxoW)) {
+
+ // We can't use AssertingIndexSearcher unfortunately since it may randomly decide to bulk
+ // score a sub-range of docs instead of all docs at once. This is incompatible will drill
+ // sideways, so we have to do our own check here. This just makes sure we call #finish on
+ // the last leaf. It's too bad we need to do this and maybe some day we can clean this up
+ // by rethinking drill-sideways:
+ IndexSearcher searcher =
+ new IndexSearcher(r) {
+ @Override
+ protected void search(
+ List<LeafReaderContext> leaves, Weight weight, Collector collector)
+ throws IOException {
+ AssertingCollector assertingCollector = AssertingCollector.wrap(collector);
+ super.search(leaves, weight, assertingCollector);
+ assert assertingCollector.hasFinishedCollectingPreviousLeaf;
+ }
+ };
+
+ Query baseQuery = new MatchAllDocsQuery();
+ DrillDownQuery ddq = new DrillDownQuery(facetsConfig, baseQuery);
+ ddq.add("foo", "bar");
+ DrillSideways drillSideways = new DrillSideways(searcher, facetsConfig, taxoR);
+
+ // It doesn't really matter what we have our collector manager do here since all we care
+ // about for this test are the assertions provided by the "asserting" collectors/leaf
+ // collectors, etc. But we have this definition already for other tests and it's easy to
+ // use, so we'll use it here and do a little extra checking:
+ SimpleCollectorManager cm =
+ new SimpleCollectorManager(10, Comparator.comparingInt(a -> a.docAndScore.doc));
+
+ DrillSideways.ConcurrentDrillSidewaysResult<List<DocAndScore>> result =
+ drillSideways.search(ddq, cm);
+
+ assertEquals(1, result.collectorResult.size());
+ }
+ }
+ }
+
private void runDrillSidewaysTestCases(FacetsConfig config, DrillSideways ds) throws Exception {
// case: drill-down on a single field; in this
// case the drill-sideways + drill-down counts ==
@@ -1490,7 +1543,6 @@ public class TestDrillSideways extends FacetTestCase {
.collect(Collectors.toList());
}
}
- ;
private int[] getTopNOrds(final int[] counts, final String[] values, int topN) {
final int[] ids = new int[counts.length];
diff --git a/lucene/test-framework/src/java/org/apache/lucene/tests/search/AssertingCollector.java b/lucene/test-framework/src/java/org/apache/lucene/tests/search/AssertingCollector.java
index af0df8cdc3f..00681aea2a0 100644
--- a/lucene/test-framework/src/java/org/apache/lucene/tests/search/AssertingCollector.java
+++ b/lucene/test-framework/src/java/org/apache/lucene/tests/search/AssertingCollector.java
@@ -24,13 +24,22 @@ import org.apache.lucene.search.FilterCollector;
import org.apache.lucene.search.LeafCollector;
import org.apache.lucene.search.Weight;
-/** A collector that asserts that it is used correctly. */
-class AssertingCollector extends FilterCollector {
+/**
+ * A collector that asserts that it is used correctly.
+ *
+ * @lucene.internal
+ */
+public class AssertingCollector extends FilterCollector {
private boolean weightSet = false;
private int maxDoc = -1;
private int previousLeafMaxDoc = 0;
- boolean hasFinishedCollectingPreviousLeaf = true;
+
+ // public visibility for drill-sideways testing, since drill-sideways can't directly use
+ // AssertingIndexSearcher
+ // TODO: this is a pretty hacky workaround. It would be nice to rethink drill-sideways (for
+ // multiple reasons) and move this back to pkg-private at some point
+ public boolean hasFinishedCollectingPreviousLeaf = true;
/** Wrap the given collector in order to add assertions. */
public static AssertingCollector wrap(Collector in) {