gf2121 commented on code in PR #12642:
URL: https://github.com/apache/lucene/pull/12642#discussion_r1357151214


##########
lucene/facet/src/test/org/apache/lucene/facet/TestDrillSideways.java:
##########
@@ -316,6 +316,58 @@ public void testBasic() throws Exception {
     IOUtils.close(searcher.getIndexReader(), taxoReader, taxoWriter, dir, 
taxoDir);
   }
 
+  public void testCollectionTerminated() 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)) {
+        IndexSearcher searcher = new IndexSearcher(r);
+
+        Query baseQuery = new MatchAllDocsQuery();
+        Query dimQ = new TermQuery(new Term("foo", "bar"));
+
+        DrillDownQuery ddq = new DrillDownQuery(facetsConfig, baseQuery);
+        ddq.add("foo", dimQ);
+        DrillSideways drillSideways = new DrillSideways(searcher, 
facetsConfig, taxoR);
+
+        CollectorManager<?, ?> cm =
+            new CollectorManager<>() {
+              @Override
+              public Collector newCollector() throws IOException {
+                return new Collector() {

Review Comment:
   Thanks @gsmiller , conflicted +1 :)
   
   I think the root cause here is that `AssertingCollector` can not be 
perfectly self-contained —— it requires `AssertingIndexSeacher` to do some 
additional check.  As we are considering to make `AssertingCollector` public, 
we should at least expose something to allow users that want to use public 
`AssertingCollector` directly without `AssertingIndexSearcher` to do the 
additional check. Maybe there's a more elegant way than exposing 
`hasFinishedCollectingPreviousLeaf` directly, but I'm not sure what it is.



##########
lucene/facet/src/test/org/apache/lucene/facet/TestDrillSideways.java:
##########
@@ -316,6 +316,58 @@ public void testBasic() throws Exception {
     IOUtils.close(searcher.getIndexReader(), taxoReader, taxoWriter, dir, 
taxoDir);
   }
 
+  public void testCollectionTerminated() 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)) {
+        IndexSearcher searcher = new IndexSearcher(r);
+
+        Query baseQuery = new MatchAllDocsQuery();
+        Query dimQ = new TermQuery(new Term("foo", "bar"));
+
+        DrillDownQuery ddq = new DrillDownQuery(facetsConfig, baseQuery);
+        ddq.add("foo", dimQ);
+        DrillSideways drillSideways = new DrillSideways(searcher, 
facetsConfig, taxoR);
+
+        CollectorManager<?, ?> cm =
+            new CollectorManager<>() {
+              @Override
+              public Collector newCollector() throws IOException {
+                return new Collector() {

Review Comment:
   Thanks @gsmiller , conflicted +1 :)
   
   I think the root cause here is that `AssertingCollector` can not be 
perfectly self-contained —— it requires `AssertingIndexSeacher` to do some 
additional check.  As we are considering to make `AssertingCollector` public, 
we should at least expose something to allow users that want to use public 
`AssertingCollector` directly without `AssertingIndexSearcher` to do the 
additional check. Maybe there's a more elegant way than exposing 
`hasFinishedCollectingPreviousLeaf` directly, but I'm not sure what it is.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org

Reply via email to