Skip to content

Commit

Permalink
SOLR-17296: Remove (broken) singlePass attempt when reRankScale + deb…
Browse files Browse the repository at this point in the history 
…ug is used, and fix underlying NPE.
  • Loading branch information
@hossman
hossman committed May 17, 2024
1 parent ad0a797 commit 6af52f3
Show file tree
Hide file tree
Showing 5 changed files with 151 additions and 91 deletions.
5 changes: 5 additions & 0 deletions solr/CHANGES.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,11 @@ Other Changes

* GITHUB#2454: Refactor preparePutOrPost method in HttpJdkSolrClient (Andy Webb)

================== 9.6.1 ==================
Bug Fixes
---------------------
* SOLR-17296: Remove (broken) singlePass attempt when reRankScale + debug is used, and fix underlying NPE. (hossman)

================== 9.6.0 ==================
New Features
---------------------
Expand Down
32 changes: 0 additions & 32 deletions solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -782,7 +781,6 @@ protected void createMainQuery(ResponseBuilder rb) {
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
Expand Down Expand Up @@ -864,36 +862,6 @@ protected void createMainQuery(ResponseBuilder rb) {
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);
Expand Down
63 changes: 44 additions & 19 deletions solr/core/src/java/org/apache/solr/search/ReRankQParserPlugin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -63,6 +65,40 @@ public QParser createParser(

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);
Expand All @@ -88,19 +124,8 @@ public Query parse() throws SyntaxError {

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;

Expand All @@ -111,15 +136,15 @@ public Query parse() throws SyntaxError {
reRankScaleWeight,
reRankOperator,
new ReRankQueryRescorer(reRankQuery, 1, ReRankOperator.REPLACE),
debugQuery);
explainResults);

if (reRankScaler.scaleScores()) {
// Scaler applies the weighting instead of the rescorer
reRankWeight = 1;
}

return new ReRankQuery(
reRankQuery, reRankDocs, reRankWeight, reRankOperator, reRankScaler, debugQuery);
reRankQuery, reRankDocs, reRankWeight, reRankOperator, reRankScaler, explainResults);
}
}

Expand Down Expand Up @@ -165,7 +190,7 @@ protected float combine(
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() {
Expand Down Expand Up @@ -198,7 +223,7 @@ public ReRankQuery(
double reRankWeight,
ReRankOperator reRankOperator,
ReRankScaler reRankScaler,
boolean debugQuery) {
boolean explainResults) {
super(
defaultQuery,
reRankDocs,
Expand All @@ -207,7 +232,7 @@ public ReRankQuery(
reRankOperator);
this.reRankQuery = reRankQuery;
this.reRankWeight = reRankWeight;
this.debugQuery = debugQuery;
this.explainResults = explainResults;
}

@Override
Expand Down Expand Up @@ -246,13 +271,13 @@ public String toString(String s) {
@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 {
Expand Down
19 changes: 13 additions & 6 deletions solr/core/src/java/org/apache/solr/search/ReRankScaler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public class ReRankScaler {
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;
Expand All @@ -45,11 +45,11 @@ public ReRankScaler(
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
Expand Down Expand Up @@ -171,12 +171,12 @@ public ScoreDoc[] scaleScores(ScoreDoc[] originalDocs, ScoreDoc[] rescoredDocs,
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;
Expand Down Expand Up @@ -345,7 +345,14 @@ public Explanation explain(
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();
Expand Down
Loading

0 comments on commit 6af52f3

Please sign in to comment.