You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/07/12 20:00:50 UTC
[GitHub] [solr] benediktarnold opened a new pull request #212: SOLR-15531: significantTerms streaming function should not fail on empty collections
benediktarnold opened a new pull request #212:
URL: https://github.com/apache/solr/pull/212
https://issues.apache.org/jira/browse/SOLR-15531
# Description
The `SignificantTermsQParserPlugin` uses the class `SparseFixedBitSet` internall that requires a length parameter >= 1. In order to fill the parameter, the result of `searcher.indexReader().maxDoc()` which is 0 for shards without any document.
# Solution
Before the Plugin creates the analytics collector, the proposed solution checks `maxDocs` and decides if it should return the `SignifcantTermsCollector` or a no op collector.
# Tests
1. A test to check the existing behavior for collections with at least one document
2. A test to verify the error against a collection with 0 documents.
# Checklist
Please review the following and check all that apply:
- [ x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ x] I have created a Jira issue and added the issue ID to my pull request title.
- [x ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x ] I have developed this patch against the `main` branch.
- [ (still running)] I have run `./gradlew check`.
- [ x] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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
[GitHub] [solr] madrob commented on a change in pull request #212: SOLR-15531: significantTerms streaming function should not fail on empty collections
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #212:
URL: https://github.com/apache/solr/pull/212#discussion_r670652916
##########
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:
No, I didn't realize it was private when I made that suggestion.
--
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
[GitHub] [solr] benediktarnold commented on a change in pull request #212: SOLR-15531: significantTerms streaming function should not fail on empty collections
Posted by GitBox <gi...@apache.org>.
benediktarnold commented on a change in pull request #212:
URL: https://github.com/apache/solr/pull/212#discussion_r668501486
##########
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:
Done.
I've found many wildcard imports in the Solr codebase. Don't you think it's worth to add an editorconfig to enforce that rule?
--
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
[GitHub] [solr] madrob merged pull request #212: SOLR-15531: significantTerms streaming function should not fail on empty collections
Posted by GitBox <gi...@apache.org>.
madrob merged pull request #212:
URL: https://github.com/apache/solr/pull/212
--
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
[GitHub] [solr] madrob commented on a change in pull request #212: SOLR-15531: significantTerms streaming function should not fail on empty collections
Posted by 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
[GitHub] [solr] benediktarnold commented on a change in pull request #212: SOLR-15531: significantTerms streaming function should not fail on empty collections
Posted by GitBox <gi...@apache.org>.
benediktarnold commented on a change in pull request #212:
URL: https://github.com/apache/solr/pull/212#discussion_r668499091
##########
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:
`SignificantTermsQuery` is a private class and can't be used in the test. Should I change the visibility?
--
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
[GitHub] [solr] benediktarnold commented on a change in pull request #212: SOLR-15531: significantTerms streaming function should not fail on empty collections
Posted by GitBox <gi...@apache.org>.
benediktarnold commented on a change in pull request #212:
URL: https://github.com/apache/solr/pull/212#discussion_r668497734
##########
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:
Unfortunately not. At least the class `org.apache.solr.client.solrj.io.stream.SignificantTermsStream` requires all attributes in the response and fails otherwise.
That's just SolrJ. I don't know the behaviour of other clients and don't want to break them.
--
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