[GitHub] [solr] madrob commented on a change in pull request #212: SOLR-15531: significantTerms streaming function should not fail on empty collections

[GitBox <gi...@apache.org>]

madrob commented on a change in pull request #212:
URL: https://github.com/apache/solr/pull/212#discussion_r668326576



##########
File path: solr/core/src/test/org/apache/solr/search/SignificantTermsQParserPluginTest.java
##########
@@ -17,19 +17,130 @@
 
 package org.apache.solr.search;
 
+import org.apache.commons.io.FileUtils;
 import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.SolrInputDocument;
+import org.apache.solr.common.params.MapSolrParams;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.request.LocalSolrQueryRequest;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.update.AddUpdateCommand;
+import org.apache.solr.update.CommitUpdateCommand;
+import org.apache.solr.update.DeleteUpdateCommand;
+import org.apache.solr.util.RefCounted;
+import org.junit.BeforeClass;
 import org.junit.Test;
 
+import java.io.File;
+import java.io.IOException;
+import java.util.*;
+
 public class SignificantTermsQParserPluginTest extends SolrTestCaseJ4 {
 
-  /**
-   * Test the backwards compatibility for a typo in the SignificantTermsQParserPlugin. It will fail if the backwards
-   * compatibility is broken.
-   */
-  @Test
-  public void testQParserBackwardsCompatibility() {
-    assertEquals("significantTerms", SignificantTermsQParserPlugin.NAME);
-    assertEquals(SignificantTermsQParserPlugin.class,
-        QParserPlugin.standardPlugins.get(SignificantTermsQParserPlugin.NAME).getClass());
-  }
+    @BeforeClass
+    public static void setUpCore() throws Exception {
+        String tmpSolrHome = createTempDir().toFile().getAbsolutePath();
+        FileUtils.copyDirectory(new File(TEST_HOME()), new File(tmpSolrHome).getAbsoluteFile());
+        initCore("solrconfig.xml", "schema.xml", new File(tmpSolrHome).getAbsolutePath());
+    }
+
+    /**
+     * Test the backwards compatibility for a typo in the SignificantTermsQParserPlugin. It will fail if the backwards
+     * compatibility is broken.
+     */
+    @Test
+    public void testQParserBackwardsCompatibility() {
+        assertEquals("significantTerms", SignificantTermsQParserPlugin.NAME);
+        assertEquals(SignificantTermsQParserPlugin.class,
+                QParserPlugin.standardPlugins.get(SignificantTermsQParserPlugin.NAME).getClass());
+    }
+
+    @Test
+    public void testEmptyCollectionDoesNotThrow() throws Exception {
+        SolrCore emptyCore = h.getCore();
+        QParserPlugin qParserPlugin = QParserPlugin.standardPlugins.get(SignificantTermsQParserPlugin.NAME);
+        Map<String, String> params = new HashMap<>();
+        params.put("field", "cat");
+        QParser parser = qParserPlugin.createParser("", new MapSolrParams(params), new MapSolrParams(new HashMap<>()), null);
+        AnalyticsQuery query = (AnalyticsQuery) parser.parse();

Review comment:
       nit: I think this would be more clear as a SignificantTermsQuery

##########
File path: solr/core/src/java/org/apache/solr/search/SignificantTermsQParserPlugin.java
##########
@@ -89,10 +87,43 @@ public SignificantTermsQuery(String field, int numTerms, float minDocs, float ma
 
     @Override
     public DelegatingCollector getAnalyticsCollector(ResponseBuilder rb, IndexSearcher searcher) {
+      if (searcher.getIndexReader().maxDoc() <= 0) {
+        return new NoOpTermsCollector(rb);
+      }
       return new SignifcantTermsCollector(rb, searcher, field, numTerms, minDocs, maxDocs, minTermLength);
     }
   }
 
+  private static class NoOpTermsCollector extends DelegatingCollector {
+    private ResponseBuilder rb;
+
+    private NoOpTermsCollector(ResponseBuilder rb) {
+      this.rb = rb;
+    }
+
+    @Override
+    public void collect(int doc) throws IOException {
+    }
+
+    @Override
+    public void finish() throws IOException {
+      List<String> outTerms = new ArrayList<>();
+      List<Integer> outFreq = new ArrayList<>();
+      List<Integer> outQueryFreq = new ArrayList<>();
+      List<Double> scores = new ArrayList<>();
+
+      LinkedHashMap<String, Object> response = new LinkedHashMap<>();
+
+      rb.rsp.add("significantTerms", response);
+
+      response.put("numDocs", 0);
+      response.put("sterms", outTerms);
+      response.put("scores", scores);
+      response.put("docFreq", outFreq);
+      response.put("queryDocFreq", outQueryFreq);

Review comment:
       once we've established that numDocs=0, do we need to include all of these in the response?

##########
File path: solr/core/src/java/org/apache/solr/search/SignificantTermsQParserPlugin.java
##########
@@ -28,9 +28,7 @@
 import org.apache.lucene.index.PostingsEnum;
 import org.apache.lucene.index.Terms;
 import org.apache.lucene.index.TermsEnum;
-import org.apache.lucene.search.DocIdSetIterator;
-import org.apache.lucene.search.IndexSearcher;
-import org.apache.lucene.search.Query;
+import org.apache.lucene.search.*;

Review comment:
       please don't use wildcard imports.




-- 
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-unsubscribe@solr.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org