Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forbid expensive query parts in ranking evaluation #30151

Merged
merged 4 commits into from
May 14, 2018
Merged

Forbid expensive query parts in ranking evaluation #30151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6084785
Forbid expensive query parts in ranking evaluation
Apr 24, 2018
f0dde56
Merge branch 'master' into rankEval-forbidQueryElements
Apr 26, 2018
8633a4b
Merge branch 'master' into rankEval-forbidQueryElements
May 14, 2018
2d94553
Add profile and explain to forbidden elements
May 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RankEvalSpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public class RankEvalSpec implements Writeable, ToXContentObject {
/** Default max number of requests. */
private static final int MAX_CONCURRENT_SEARCHES = 10;
/** optional: Templates to base test requests on */
private Map<String, Script> templates = new HashMap<>();
private final Map<String, Script> templates = new HashMap<>();

public RankEvalSpec(List<RatedRequest> ratedRequests, EvaluationMetric metric, Collection<ScriptWithId> templates) {
this.metric = Objects.requireNonNull(metric, "Cannot evaluate ranking if no evaluation metric is provided.");
Expand All @@ -68,8 +68,8 @@ public RankEvalSpec(List<RatedRequest> ratedRequests, EvaluationMetric metric, C
this.ratedRequests = ratedRequests;
if (templates == null || templates.isEmpty()) {
for (RatedRequest request : ratedRequests) {
if (request.getTestRequest() == null) {
throw new IllegalStateException("Cannot evaluate ranking if neither template nor test request is "
if (request.getEvaluationRequest() == null) {
throw new IllegalStateException("Cannot evaluate ranking if neither template nor evaluation request is "
+ "provided. Seen for request id: " + request.getId());
}
}
Expand Down
101 changes: 69 additions & 32 deletions modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RatedRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,12 @@ public class RatedRequest implements Writeable, ToXContentObject {
private final String id;
private final List<String> summaryFields;
private final List<RatedDocument> ratedDocs;
// Search request to execute for this rated request. This can be null if template and corresponding parameters are supplied.
/**
* Search request to execute for this rated request. This can be null in
* case the query is supplied as a template with corresponding parameters
*/
@Nullable
private SearchSourceBuilder testRequest;
private final SearchSourceBuilder evaluationRequest;
/**
* Map of parameters to use for filling a query template, can be used
* instead of providing testRequest.
Expand All @@ -86,27 +89,49 @@ public class RatedRequest implements Writeable, ToXContentObject {
@Nullable
private String templateId;

private RatedRequest(String id, List<RatedDocument> ratedDocs, SearchSourceBuilder testRequest,
/**
* Create a rated request with template ids and parameters.
*
* @param id a unique name for this rated request
* @param ratedDocs a list of document ratings
* @param params template parameters
* @param templateId a templare id
*/
public RatedRequest(String id, List<RatedDocument> ratedDocs, Map<String, Object> params,
String templateId) {
this(id, ratedDocs, null, params, templateId);
}

/**
* Create a rated request using a {@link SearchSourceBuilder} to define the
* evaluated query.
*
* @param id a unique name for this rated request
* @param ratedDocs a list of document ratings
* @param evaluatedQuery the query that is evaluated
*/
public RatedRequest(String id, List<RatedDocument> ratedDocs, SearchSourceBuilder evaluatedQuery) {
this(id, ratedDocs, evaluatedQuery, new HashMap<>(), null);
}

private RatedRequest(String id, List<RatedDocument> ratedDocs, SearchSourceBuilder evaluatedQuery,
Map<String, Object> params, String templateId) {
if (params != null && (params.size() > 0 && testRequest != null)) {
if (params != null && (params.size() > 0 && evaluatedQuery != null)) {
throw new IllegalArgumentException(
"Ambiguous rated request: Set both, verbatim test request and test request "
+ "template parameters.");
"Ambiguous rated request: Set both, verbatim test request and test request " + "template parameters.");
}
if (templateId != null && testRequest != null) {
if (templateId != null && evaluatedQuery != null) {
throw new IllegalArgumentException(
"Ambiguous rated request: Set both, verbatim test request and test request "
+ "template parameters.");
"Ambiguous rated request: Set both, verbatim test request and test request " + "template parameters.");
}
if ((params == null || params.size() < 1) && testRequest == null) {
throw new IllegalArgumentException(
"Need to set at least test request or test request template parameters.");
if ((params == null || params.size() < 1) && evaluatedQuery == null) {
throw new IllegalArgumentException("Need to set at least test request or test request template parameters.");
}
if ((params != null && params.size() > 0) && templateId == null) {
throw new IllegalArgumentException(
"If template parameters are supplied need to set id of template to apply "
+ "them to too.");
throw new IllegalArgumentException("If template parameters are supplied need to set id of template to apply " + "them to too.");
}
validateEvaluatedQuery(evaluatedQuery);

// check that not two documents with same _index/id are specified
Set<DocumentKey> docKeys = new HashSet<>();
for (RatedDocument doc : ratedDocs) {
Expand All @@ -118,7 +143,7 @@ private RatedRequest(String id, List<RatedDocument> ratedDocs, SearchSourceBuild
}

this.id = id;
this.testRequest = testRequest;
this.evaluationRequest = evaluatedQuery;
this.ratedDocs = new ArrayList<>(ratedDocs);
if (params != null) {
this.params = new HashMap<>(params);
Expand All @@ -129,18 +154,30 @@ private RatedRequest(String id, List<RatedDocument> ratedDocs, SearchSourceBuild
this.summaryFields = new ArrayList<>();
}

public RatedRequest(String id, List<RatedDocument> ratedDocs, Map<String, Object> params,
String templateId) {
this(id, ratedDocs, null, params, templateId);
}

public RatedRequest(String id, List<RatedDocument> ratedDocs, SearchSourceBuilder testRequest) {
this(id, ratedDocs, testRequest, new HashMap<>(), null);
static void validateEvaluatedQuery(SearchSourceBuilder evaluationRequest) {
// ensure that testRequest, if set, does not contain aggregation, suggest or highlighting section
if (evaluationRequest != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if profile and explain should be forbidden too? Both have non-negligible impact on performance, and seem irrelevant to ranking as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I think I will add that before I merge. Thanks.

if (evaluationRequest.suggest() != null) {
throw new IllegalArgumentException("Query in rated requests should not contain a suggest section.");
}
if (evaluationRequest.aggregations() != null) {
throw new IllegalArgumentException("Query in rated requests should not contain aggregations.");
}
if (evaluationRequest.highlighter() != null) {
throw new IllegalArgumentException("Query in rated requests should not contain a highlighter section.");
}
if (evaluationRequest.explain() != null && evaluationRequest.explain()) {
throw new IllegalArgumentException("Query in rated requests should not use explain.");
}
if (evaluationRequest.profile()) {
throw new IllegalArgumentException("Query in rated requests should not use profile.");
}
}
}

public RatedRequest(StreamInput in) throws IOException {
RatedRequest(StreamInput in) throws IOException {
this.id = in.readString();
testRequest = in.readOptionalWriteable(SearchSourceBuilder::new);
evaluationRequest = in.readOptionalWriteable(SearchSourceBuilder::new);

int intentSize = in.readInt();
ratedDocs = new ArrayList<>(intentSize);
Expand All @@ -159,7 +196,7 @@ public RatedRequest(StreamInput in) throws IOException {
@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeString(id);
out.writeOptionalWriteable(testRequest);
out.writeOptionalWriteable(evaluationRequest);

out.writeInt(ratedDocs.size());
for (RatedDocument ratedDoc : ratedDocs) {
Expand All @@ -173,8 +210,8 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeOptionalString(this.templateId);
}

public SearchSourceBuilder getTestRequest() {
return testRequest;
public SearchSourceBuilder getEvaluationRequest() {
return evaluationRequest;
}

/** return the user supplied request id */
Expand Down Expand Up @@ -240,8 +277,8 @@ public static RatedRequest fromXContent(XContentParser parser) {
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
builder.field(ID_FIELD.getPreferredName(), this.id);
if (testRequest != null) {
builder.field(REQUEST_FIELD.getPreferredName(), this.testRequest);
if (evaluationRequest != null) {
builder.field(REQUEST_FIELD.getPreferredName(), this.evaluationRequest);
}
builder.startArray(RATINGS_FIELD.getPreferredName());
for (RatedDocument doc : this.ratedDocs) {
Expand Down Expand Up @@ -285,7 +322,7 @@ public final boolean equals(Object obj) {

RatedRequest other = (RatedRequest) obj;

return Objects.equals(id, other.id) && Objects.equals(testRequest, other.testRequest)
return Objects.equals(id, other.id) && Objects.equals(evaluationRequest, other.evaluationRequest)
&& Objects.equals(summaryFields, other.summaryFields)
&& Objects.equals(ratedDocs, other.ratedDocs)
&& Objects.equals(params, other.params)
Expand All @@ -294,7 +331,7 @@ public final boolean equals(Object obj) {

@Override
public final int hashCode() {
return Objects.hash(id, testRequest, summaryFields, ratedDocs, params,
return Objects.hash(id, evaluationRequest, summaryFields, ratedDocs, params,
templateId);
}
}
17 changes: 10 additions & 7 deletions ...les/rank-eval/src/main/java/org/elasticsearch/index/rankeval/TransportRankEvalAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import java.util.concurrent.ConcurrentHashMap;

import static org.elasticsearch.common.xcontent.XContentHelper.createParser;
import static org.elasticsearch.index.rankeval.RatedRequest.validateEvaluatedQuery;

/**
* Instances of this class execute a collection of search intents (read: user
Expand Down Expand Up @@ -99,15 +100,17 @@ protected void doExecute(RankEvalRequest request, ActionListener<RankEvalRespons
msearchRequest.maxConcurrentSearchRequests(evaluationSpecification.getMaxConcurrentSearches());
List<RatedRequest> ratedRequestsInSearch = new ArrayList<>();
for (RatedRequest ratedRequest : ratedRequests) {
SearchSourceBuilder ratedSearchSource = ratedRequest.getTestRequest();
if (ratedSearchSource == null) {
SearchSourceBuilder evaluationRequest = ratedRequest.getEvaluationRequest();
if (evaluationRequest == null) {
Map<String, Object> params = ratedRequest.getParams();
String templateId = ratedRequest.getTemplateId();
TemplateScript.Factory templateScript = scriptsWithoutParams.get(templateId);
String resolvedRequest = templateScript.newInstance(params).execute();
try (XContentParser subParser = createParser(namedXContentRegistry,
LoggingDeprecationHandler.INSTANCE, new BytesArray(resolvedRequest), XContentType.JSON)) {
ratedSearchSource = SearchSourceBuilder.fromXContent(subParser, false);
evaluationRequest = SearchSourceBuilder.fromXContent(subParser, false);
// check for parts that should not be part of a ranking evaluation request
validateEvaluatedQuery(evaluationRequest);
} catch (IOException e) {
// if we fail parsing, put the exception into the errors map and continue
errors.put(ratedRequest.getId(), e);
Expand All @@ -116,17 +119,17 @@ LoggingDeprecationHandler.INSTANCE, new BytesArray(resolvedRequest), XContentTyp
}

if (metric.forcedSearchSize().isPresent()) {
ratedSearchSource.size(metric.forcedSearchSize().get());
evaluationRequest.size(metric.forcedSearchSize().get());
}

ratedRequestsInSearch.add(ratedRequest);
List<String> summaryFields = ratedRequest.getSummaryFields();
if (summaryFields.isEmpty()) {
ratedSearchSource.fetchSource(false);
evaluationRequest.fetchSource(false);
} else {
ratedSearchSource.fetchSource(summaryFields.toArray(new String[summaryFields.size()]), new String[0]);
evaluationRequest.fetchSource(summaryFields.toArray(new String[summaryFields.size()]), new String[0]);
}
SearchRequest searchRequest = new SearchRequest(request.indices(), ratedSearchSource);
SearchRequest searchRequest = new SearchRequest(request.indices(), evaluationRequest);
searchRequest.indicesOptions(request.indicesOptions());
msearchRequest.add(searchRequest);
}
Expand Down
60 changes: 51 additions & 9 deletions modules/rank-eval/src/test/java/org/elasticsearch/index/rankeval/RatedRequestsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,11 @@
import org.elasticsearch.index.query.MatchAllQueryBuilder;
import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.search.SearchModule;
import org.elasticsearch.search.aggregations.AggregationBuilders;
import org.elasticsearch.search.builder.SearchSourceBuilder;
import org.elasticsearch.search.fetch.subphase.highlight.HighlightBuilder;
import org.elasticsearch.search.suggest.SuggestBuilder;
import org.elasticsearch.search.suggest.SuggestBuilders;
import org.elasticsearch.test.ESTestCase;
import org.junit.AfterClass;
import org.junit.BeforeClass;
Expand Down Expand Up @@ -165,7 +169,7 @@ public void testEqualsAndHash() throws IOException {

private static RatedRequest mutateTestItem(RatedRequest original) {
String id = original.getId();
SearchSourceBuilder testRequest = original.getTestRequest();
SearchSourceBuilder evaluationRequest = original.getEvaluationRequest();
List<RatedDocument> ratedDocs = original.getRatedDocs();
Map<String, Object> params = original.getParams();
List<String> summaryFields = original.getSummaryFields();
Expand All @@ -177,11 +181,11 @@ private static RatedRequest mutateTestItem(RatedRequest original) {
id = randomValueOtherThan(id, () -> randomAlphaOfLength(10));
break;
case 1:
if (testRequest != null) {
int size = randomValueOtherThan(testRequest.size(), () -> randomInt(Integer.MAX_VALUE));
testRequest = new SearchSourceBuilder();
testRequest.size(size);
testRequest.query(new MatchAllQueryBuilder());
if (evaluationRequest != null) {
int size = randomValueOtherThan(evaluationRequest.size(), () -> randomInt(Integer.MAX_VALUE));
evaluationRequest = new SearchSourceBuilder();
evaluationRequest.size(size);
evaluationRequest.query(new MatchAllQueryBuilder());
} else {
if (randomBoolean()) {
Map<String, Object> mutated = new HashMap<>();
Expand All @@ -204,10 +208,10 @@ private static RatedRequest mutateTestItem(RatedRequest original) {
}

RatedRequest ratedRequest;
if (testRequest == null) {
if (evaluationRequest == null) {
ratedRequest = new RatedRequest(id, ratedDocs, params, templateId);
} else {
ratedRequest = new RatedRequest(id, ratedDocs, testRequest);
ratedRequest = new RatedRequest(id, ratedDocs, evaluationRequest);
}
ratedRequest.addSummaryFields(summaryFields);

Expand Down Expand Up @@ -258,6 +262,44 @@ public void testSettingTemplateIdNoParamsThrows() {
expectThrows(IllegalArgumentException.class, () -> new RatedRequest("id", ratedDocs, null, "templateId"));
}

public void testAggsNotAllowed() {
List<RatedDocument> ratedDocs = Arrays.asList(new RatedDocument("index1", "id1", 1));
SearchSourceBuilder query = new SearchSourceBuilder();
query.aggregation(AggregationBuilders.terms("fieldName"));
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> new RatedRequest("id", ratedDocs, query));
assertEquals("Query in rated requests should not contain aggregations.", e.getMessage());
}

public void testSuggestionsNotAllowed() {
List<RatedDocument> ratedDocs = Arrays.asList(new RatedDocument("index1", "id1", 1));
SearchSourceBuilder query = new SearchSourceBuilder();
query.suggest(new SuggestBuilder().addSuggestion("id", SuggestBuilders.completionSuggestion("fieldname")));
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> new RatedRequest("id", ratedDocs, query));
assertEquals("Query in rated requests should not contain a suggest section.", e.getMessage());
}

public void testHighlighterNotAllowed() {
List<RatedDocument> ratedDocs = Arrays.asList(new RatedDocument("index1", "id1", 1));
SearchSourceBuilder query = new SearchSourceBuilder();
query.highlighter(new HighlightBuilder().field("field"));
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> new RatedRequest("id", ratedDocs, query));
assertEquals("Query in rated requests should not contain a highlighter section.", e.getMessage());
}

public void testExplainNotAllowed() {
List<RatedDocument> ratedDocs = Arrays.asList(new RatedDocument("index1", "id1", 1));
IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
() -> new RatedRequest("id", ratedDocs, new SearchSourceBuilder().explain(true)));
assertEquals("Query in rated requests should not use explain.", e.getMessage());
}

public void testProfileNotAllowed() {
List<RatedDocument> ratedDocs = Arrays.asList(new RatedDocument("index1", "id1", 1));
IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
() -> new RatedRequest("id", ratedDocs, new SearchSourceBuilder().profile(true)));
assertEquals("Query in rated requests should not use profile.", e.getMessage());
}

/**
* test that modifying the order of index/docId to make sure it doesn't
* matter for parsing xContent
Expand Down Expand Up @@ -287,7 +329,7 @@ public void testParseFromXContent() throws IOException {
try (XContentParser parser = createParser(JsonXContent.jsonXContent, querySpecString)) {
RatedRequest specification = RatedRequest.fromXContent(parser);
assertEquals("my_qa_query", specification.getId());
assertNotNull(specification.getTestRequest());
assertNotNull(specification.getEvaluationRequest());
List<RatedDocument> ratedDocs = specification.getRatedDocs();
assertEquals(3, ratedDocs.size());
for (int i = 0; i < 3; i++) {
Expand Down
Loading