SOLR-17296: Remove (broken) singlePass attempt when reRankScale + debug is used, and fix underlying NPE.
(cherry picked from commit 6af52f33ac78155523ec6fc610c563127b81c139)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 00a2322..cb3e966 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -16,6 +16,8 @@
* SOLR-17275: SolrJ ZkClientClusterStateProvider, revert SOLR-17153 for perf regression when aliases are used. (Aparna Suresh)
+* SOLR-17296: Remove (broken) singlePass attempt when reRankScale + debug is used, and fix underlying NPE. (hossman)
+
Dependency Upgrades
---------------------
(No changes)
diff --git a/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java b/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
index 4f17cb7..bb364f8 100644
--- a/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
+++ b/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
@@ -89,7 +89,6 @@
import org.apache.solr.search.QueryResult;
import org.apache.solr.search.QueryUtils;
import org.apache.solr.search.RankQuery;
-import org.apache.solr.search.ReRankQParserPlugin;
import org.apache.solr.search.ReturnFields;
import org.apache.solr.search.SolrIndexSearcher;
import org.apache.solr.search.SolrReturnFields;
@@ -779,7 +778,6 @@
boolean distribSinglePass = rb.req.getParams().getBool(ShardParams.DISTRIB_SINGLE_PASS, false);
if (distribSinglePass
- || singlePassExplain(rb.req.getParams())
|| (fields != null
&& fields.wantsField(keyFieldName)
&& fields.getRequestedFieldNames() != null
@@ -861,36 +859,6 @@
rb.addRequest(this, sreq);
}
- private boolean singlePassExplain(SolrParams params) {
-
- /*
- * Currently there is only one explain that requires a single pass
- * and that is the reRank when scaling is used.
- */
-
- String rankQuery = params.get(CommonParams.RQ);
- if (rankQuery != null) {
- if (rankQuery.contains(ReRankQParserPlugin.RERANK_MAIN_SCALE)
- || rankQuery.contains(ReRankQParserPlugin.RERANK_SCALE)) {
- boolean debugQuery = params.getBool(CommonParams.DEBUG_QUERY, false);
- if (debugQuery) {
- return true;
- } else {
- String[] debugParams = params.getParams(CommonParams.DEBUG);
- if (debugParams != null) {
- for (String debugParam : debugParams) {
- if (debugParam.equals("true")) {
- return true;
- }
- }
- }
- }
- }
- }
-
- return false;
- }
-
protected boolean addFL(StringBuilder fl, String field, boolean additionalAdded) {
if (additionalAdded) fl.append(",");
fl.append(field);
diff --git a/solr/core/src/java/org/apache/solr/search/ReRankQParserPlugin.java b/solr/core/src/java/org/apache/solr/search/ReRankQParserPlugin.java
index e9597c3..5bf5ce4 100644
--- a/solr/core/src/java/org/apache/solr/search/ReRankQParserPlugin.java
+++ b/solr/core/src/java/org/apache/solr/search/ReRankQParserPlugin.java
@@ -25,7 +25,9 @@
import org.apache.solr.common.params.CommonParams;
import org.apache.solr.common.params.SolrParams;
import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.handler.component.ResponseBuilder;
import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.request.SolrRequestInfo;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -63,6 +65,40 @@
private static class ReRankQParser extends QParser {
+ private boolean isExplainResults() {
+ final SolrRequestInfo ri = SolrRequestInfo.getRequestInfo();
+ if (null != ri) {
+ final ResponseBuilder rb = ri.getResponseBuilder();
+ if (null != rb) {
+ return rb.isDebugResults();
+ }
+ }
+
+ // HACK: The code below should not be used. It is preserved for backcompat
+ // on the slim remote chance that someone is using ReRankQParserPlugin
+ // w/o using SearchHandler+ResponseBuilder
+ //
+ // (It's also wrong, and doesn't account for thigns like debug=true
+ // or debug=all ... but as stated: it's for esoteric backcompat purposes
+ // only, so we're not going to change it and start returning "true"
+ // if existing code doesn't expect it
+
+ boolean debugQuery = params.getBool(CommonParams.DEBUG_QUERY, false);
+
+ if (!debugQuery) {
+ String[] debugParams = params.getParams(CommonParams.DEBUG);
+ if (debugParams != null) {
+ for (String debugParam : debugParams) {
+ if ("true".equals(debugParam)) {
+ debugQuery = true;
+ break;
+ }
+ }
+ }
+ }
+ return debugQuery;
+ }
+
public ReRankQParser(
String query, SolrParams localParams, SolrParams params, SolrQueryRequest req) {
super(query, localParams, params, req);
@@ -88,19 +124,8 @@
String mainScale = localParams.get(RERANK_MAIN_SCALE);
String reRankScale = localParams.get(RERANK_SCALE);
- boolean debugQuery = params.getBool(CommonParams.DEBUG_QUERY, false);
- if (!debugQuery) {
- String[] debugParams = params.getParams(CommonParams.DEBUG);
- if (debugParams != null) {
- for (String debugParam : debugParams) {
- if ("true".equals(debugParam)) {
- debugQuery = true;
- break;
- }
- }
- }
- }
+ final boolean explainResults = isExplainResults();
double reRankScaleWeight = reRankWeight;
@@ -111,7 +136,7 @@
reRankScaleWeight,
reRankOperator,
new ReRankQueryRescorer(reRankQuery, 1, ReRankOperator.REPLACE),
- debugQuery);
+ explainResults);
if (reRankScaler.scaleScores()) {
// Scaler applies the weighting instead of the rescorer
@@ -119,7 +144,7 @@
}
return new ReRankQuery(
- reRankQuery, reRankDocs, reRankWeight, reRankOperator, reRankScaler, debugQuery);
+ reRankQuery, reRankDocs, reRankWeight, reRankOperator, reRankScaler, explainResults);
}
}
@@ -165,7 +190,7 @@
private static final class ReRankQuery extends AbstractReRankQuery {
private final Query reRankQuery;
private final double reRankWeight;
- private final boolean debugQuery;
+ private final boolean explainResults;
@Override
public int hashCode() {
@@ -198,7 +223,7 @@
double reRankWeight,
ReRankOperator reRankOperator,
ReRankScaler reRankScaler,
- boolean debugQuery) {
+ boolean explainResults) {
super(
defaultQuery,
reRankDocs,
@@ -207,7 +232,7 @@
reRankOperator);
this.reRankQuery = reRankQuery;
this.reRankWeight = reRankWeight;
- this.debugQuery = debugQuery;
+ this.explainResults = explainResults;
}
@Override
@@ -246,13 +271,13 @@
@Override
protected Query rewrite(Query rewrittenMainQuery) throws IOException {
return new ReRankQuery(
- reRankQuery, reRankDocs, reRankWeight, reRankOperator, reRankScaler, debugQuery)
+ reRankQuery, reRankDocs, reRankWeight, reRankOperator, reRankScaler, explainResults)
.wrap(rewrittenMainQuery);
}
@Override
public boolean getCache() {
- if (reRankScaler.scaleScores() && debugQuery) {
+ if (reRankScaler.scaleScores() && explainResults) {
// Caching breaks explain when reRankScaling is used.
return false;
} else {
diff --git a/solr/core/src/java/org/apache/solr/search/ReRankScaler.java b/solr/core/src/java/org/apache/solr/search/ReRankScaler.java
index 686faa4..8410467 100644
--- a/solr/core/src/java/org/apache/solr/search/ReRankScaler.java
+++ b/solr/core/src/java/org/apache/solr/search/ReRankScaler.java
@@ -32,7 +32,7 @@
protected int mainQueryMax = -1;
protected int reRankQueryMin = -1;
protected int reRankQueryMax = -1;
- protected boolean debugQuery;
+ protected boolean explainResults;
protected ReRankOperator reRankOperator;
protected ReRankScalerExplain reRankScalerExplain;
private QueryRescorer replaceRescorer;
@@ -45,11 +45,11 @@
double reRankScaleWeight,
ReRankOperator reRankOperator,
QueryRescorer replaceRescorer,
- boolean debugQuery)
+ boolean explainResults)
throws SyntaxError {
this.reRankScaleWeight = reRankScaleWeight;
- this.debugQuery = debugQuery;
+ this.explainResults = explainResults;
this.reRankScalerExplain = new ReRankScalerExplain(mainScale, reRankScale);
this.replaceRescorer = replaceRescorer;
if (reRankOperator != ReRankOperator.ADD
@@ -171,12 +171,12 @@
scaledOriginalScoreMap = originalScoreMap;
}
- this.reRankSet = debugQuery ? new HashSet<>() : null;
+ this.reRankSet = explainResults ? new HashSet<>() : null;
for (int i = 0; i < howMany; i++) {
ScoreDoc rescoredDoc = rescoredDocs[i];
int doc = rescoredDoc.doc;
- if (debugQuery) {
+ if (explainResults) {
reRankSet.add(doc);
}
float score = rescoredDoc.score;
@@ -345,7 +345,14 @@
int doc, Explanation mainQueryExplain, Explanation reRankQueryExplain) {
float reRankScore = reRankQueryExplain.getDetails()[1].getValue().floatValue();
float mainScore = mainQueryExplain.getValue().floatValue();
- if (reRankSet.contains(doc)) {
+ if (null == reRankSet) {
+ // we don't have the data needed to accurately report scaling,
+ // probably due to distributed request
+ return Explanation.match(
+ reRankScore,
+ "ReRank Scaling effects unkown, consider using distrib.singlePass=true (see https://issues.apache.org/jira/browse/SOLR-17299)",
+ reRankQueryExplain);
+ } else if (reRankSet.contains(doc)) {
if (scaleMainScores() && scaleReRankScores()) {
if (reRankScore > 0) {
MinMaxExplain mainScaleExplain = reRankScalerExplain.getMainScaleExplain();
diff --git a/solr/core/src/test/org/apache/solr/search/DistributedReRankExplainTest.java b/solr/core/src/test/org/apache/solr/search/DistributedReRankExplainTest.java
index defe6fe..63343d3 100644
--- a/solr/core/src/test/org/apache/solr/search/DistributedReRankExplainTest.java
+++ b/solr/core/src/test/org/apache/solr/search/DistributedReRankExplainTest.java
@@ -17,9 +17,11 @@
package org.apache.solr.search;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.not;
+
import java.lang.invoke.MethodHandles;
import java.util.Map;
-import java.util.Random;
import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.client.solrj.impl.CloudSolrClient;
import org.apache.solr.client.solrj.request.CollectionAdminRequest;
@@ -29,7 +31,8 @@
import org.apache.solr.cloud.SolrCloudTestCase;
import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.common.params.CommonParams;
-import org.apache.solr.common.params.ModifiableSolrParams;
+import org.apache.solr.common.params.ShardParams;
+import org.apache.solr.common.params.SolrParams;
import org.junit.BeforeClass;
import org.junit.Test;
import org.slf4j.Logger;
@@ -61,10 +64,7 @@
.process(cluster.getSolrClient());
cluster.waitForActiveCollection(collection, 2, 2);
- }
- @Test
- public void testReRankExplain() throws Exception {
CloudSolrClient client = cluster.getSolrClient();
UpdateRequest updateRequest = new UpdateRequest();
for (int i = 0; i < 100; i++) {
@@ -75,35 +75,90 @@
}
updateRequest.process(client, COLLECTIONORALIAS);
client.commit(COLLECTIONORALIAS);
+ }
- String[] debugParams = {CommonParams.DEBUG, CommonParams.DEBUG_QUERY};
- Random random = random();
- ModifiableSolrParams solrParams = new ModifiableSolrParams();
- String reRank = "{!rerank reRankDocs=10 reRankMainScale=0-10 reRankQuery='test_s:hello'}";
- solrParams
- .add("q", "test_s:hello")
- .add(debugParams[random.nextInt(2)], "true")
- .add(CommonParams.RQ, reRank);
- QueryRequest queryRequest = new QueryRequest(solrParams);
- QueryResponse queryResponse = queryRequest.process(client, COLLECTIONORALIAS);
- Map<String, Object> debug = queryResponse.getDebugMap();
- assertNotNull(debug);
- String explain = debug.get("explain").toString();
- assertTrue(
- explain.contains("5.0101576 = combined scaled first and unscaled second pass score "));
+ @Test
+ public void testDebugTrue() throws Exception {
+ doTestReRankExplain(params(CommonParams.DEBUG_QUERY, "true"));
+ doTestReRankExplain(params(CommonParams.DEBUG, "true"));
+ }
- solrParams = new ModifiableSolrParams();
- reRank = "{!rerank reRankDocs=10 reRankScale=0-10 reRankQuery='test_s:hello'}";
- solrParams
- .add("q", "test_s:hello")
- .add(debugParams[random.nextInt(2)], "true")
- .add(CommonParams.RQ, reRank);
- queryRequest = new QueryRequest(solrParams);
- queryResponse = queryRequest.process(client, COLLECTIONORALIAS);
- debug = queryResponse.getDebugMap();
- assertNotNull(debug);
- explain = debug.get("explain").toString();
- assertTrue(
- explain.contains("10.005078 = combined unscaled first and scaled second pass score "));
+ @Test
+ public void testDebugAll() throws Exception {
+ doTestReRankExplain(params(CommonParams.DEBUG, "all"));
+ }
+
+ @Test
+ public void testDebugResults() throws Exception {
+ doTestReRankExplain(params(CommonParams.DEBUG, CommonParams.RESULTS));
+ }
+
+ private void doTestReRankExplain(final SolrParams debugParams) throws Exception {
+ final String reRankMainScale =
+ "{!rerank reRankDocs=10 reRankMainScale=0-10 reRankQuery='test_s:hello'}";
+ final String reRankScale =
+ "{!rerank reRankDocs=10 reRankScale=0-10 reRankQuery='test_s:hello'}";
+
+ { // multi-pass reRankMainScale
+ final QueryResponse queryResponse =
+ doQueryAndCommonChecks(
+ SolrParams.wrapDefaults(params(CommonParams.RQ, reRankMainScale), debugParams));
+ final Map<String, Object> debug = queryResponse.getDebugMap();
+ assertNotNull(debug);
+ final String explain = debug.get("explain").toString();
+ assertThat(explain, containsString("ReRank Scaling effects unkown"));
+ }
+
+ { // single-pass reRankMainScale
+ final QueryResponse queryResponse =
+ doQueryAndCommonChecks(
+ SolrParams.wrapDefaults(
+ params(CommonParams.RQ, reRankMainScale, ShardParams.DISTRIB_SINGLE_PASS, "true"),
+ debugParams));
+ final Map<String, Object> debug = queryResponse.getDebugMap();
+ assertNotNull(debug);
+ final String explain = debug.get("explain").toString();
+ assertThat(
+ explain,
+ containsString("5.0101576 = combined scaled first and unscaled second pass score "));
+ assertThat(explain, not(containsString("ReRank Scaling effects unkown")));
+ }
+
+ { // multi-pass reRankMainScale
+ final QueryResponse queryResponse =
+ doQueryAndCommonChecks(
+ SolrParams.wrapDefaults(params(CommonParams.RQ, reRankScale), debugParams));
+ final Map<String, Object> debug = queryResponse.getDebugMap();
+ assertNotNull(debug);
+ final String explain = debug.get("explain").toString();
+ assertThat(explain, containsString("ReRank Scaling effects unkown"));
+ }
+
+ { // single-pass reRankMainScale
+ final QueryResponse queryResponse =
+ doQueryAndCommonChecks(
+ SolrParams.wrapDefaults(
+ params(CommonParams.RQ, reRankScale, ShardParams.DISTRIB_SINGLE_PASS, "true"),
+ debugParams));
+ final Map<String, Object> debug = queryResponse.getDebugMap();
+ assertNotNull(debug);
+ final String explain = debug.get("explain").toString();
+ assertThat(
+ explain,
+ containsString("10.005078 = combined unscaled first and scaled second pass score "));
+ assertThat(explain, not(containsString("ReRank Scaling effects unkown")));
+ }
+ }
+
+ private QueryResponse doQueryAndCommonChecks(final SolrParams params) throws Exception {
+ final CloudSolrClient client = cluster.getSolrClient();
+ final QueryRequest queryRequest =
+ new QueryRequest(
+ SolrParams.wrapDefaults(
+ params, params(CommonParams.Q, "test_s:hello", "fl", "id,test_s,score")));
+
+ final QueryResponse queryResponse = queryRequest.process(client, COLLECTIONORALIAS);
+ assertNotNull(queryResponse.getResults().get(0).getFieldValue("test_s"));
+ return queryResponse;
}
}
