Skip to content

Commit

Permalink
Do not cache script queries.
Browse files Browse the repository at this point in the history
The cache relies on the equals() method so we just need to make sure script
queries can never be equals, even to themselves in the case that a weight
is used to produce a Scorer on the same segment multiple times.

Closes elastic#20763
  • Loading branch information
jpountz committed Oct 7, 2016
1 parent 194a6b1 commit efd2722
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 6 deletions.
Expand Up @@ -158,17 +158,23 @@ public String toString(String field) {

@Override
public boolean equals(Object obj) {
if (this == obj)
// TODO: Do this if/when we can assume scripts are pure functions
// and they have a reliable equals impl
/*if (this == obj)
return true;
if (sameClassAs(obj) == false)
return false;
ScriptQuery other = (ScriptQuery) obj;
return Objects.equals(script, other.script);
return Objects.equals(script, other.script);*/
return false;
}

@Override
public int hashCode() {
return Objects.hash(classHash(), script);
// TODO: Do this if/when we can assume scripts are pure functions
// and they have a reliable equals impl
// return Objects.hash(classHash(), script);
return System.identityHashCode(this);
}

@Override
Expand Down
Expand Up @@ -41,9 +41,16 @@ protected ScriptQueryBuilder doCreateTestQueryBuilder() {
return new ScriptQueryBuilder(new Script(script, ScriptType.INLINE, MockScriptEngine.NAME, params));
}

@Override
protected boolean builderGeneratesCacheableQueries() {
return false;
}

@Override
protected void doAssertLuceneQuery(ScriptQueryBuilder queryBuilder, Query query, SearchContext context) throws IOException {
assertThat(query, instanceOf(ScriptQueryBuilder.ScriptQuery.class));
// make sure queries are never equal to themselves so that they do not get cached
assertFalse(query.equals(query));
}

public void testIllegalConstructorArg() {
Expand Down Expand Up @@ -95,4 +102,5 @@ protected Set<String> getObjectsHoldingArbitraryContent() {
protected boolean isCachable(ScriptQueryBuilder queryBuilder) {
return false;
}

}
Expand Up @@ -572,6 +572,13 @@ private static QueryBuilder parseQuery(XContentParser parser, ParseFieldMatcher
return parseInnerQueryBuilder;
}

/**
* Whether the tested query builder returns queries that are cacheable.
*/
protected boolean builderGeneratesCacheableQueries() {
return true;
}

/**
* Test creates the {@link Query} from the {@link QueryBuilder} under test and delegates the
* assertions being made on the result to the implementing subclass.
Expand Down Expand Up @@ -618,8 +625,10 @@ public void testToQuery() throws IOException {
assertNotNull("toQuery should not return null", secondLuceneQuery);
assertLuceneQuery(secondQuery, secondLuceneQuery, searchContext);

assertEquals("two equivalent query builders lead to different lucene queries",
rewrite(secondLuceneQuery), rewrite(firstLuceneQuery));
if (builderGeneratesCacheableQueries()) {
assertEquals("two equivalent query builders lead to different lucene queries",
rewrite(secondLuceneQuery), rewrite(firstLuceneQuery));
}

if (supportsBoostAndQueryName()) {
secondQuery.boost(firstQuery.boost() + 1f + randomFloat());
Expand Down Expand Up @@ -663,7 +672,7 @@ protected boolean supportsBoostAndQueryName() {
* {@link #doAssertLuceneQuery(AbstractQueryBuilder, Query, SearchContext)} for query specific checks.
*/
private void assertLuceneQuery(QB queryBuilder, Query query, SearchContext context) throws IOException {
if (queryBuilder.queryName() != null) {
if (queryBuilder.queryName() != null && builderGeneratesCacheableQueries()) {
Query namedQuery = context.getQueryShardContext().copyNamedQueries().get(queryBuilder.queryName());
assertThat(namedQuery, equalTo(query));
}
Expand Down

0 comments on commit efd2722

Please sign in to comment.