Do not cache script queries. (#20799)

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 #20763
This commit is contained in:
Adrien Grand 2016-10-11 09:17:21 +02:00 committed by GitHub
parent 971b7ec542
commit 1914df7b5f
3 changed files with 33 additions and 8 deletions

View File

@ -138,9 +138,8 @@ public class ScriptQueryBuilder extends AbstractQueryBuilder<ScriptQueryBuilder>
static class ScriptQuery extends Query {
private final Script script;
private final SearchScript searchScript;
final Script script;
final SearchScript searchScript;
public ScriptQuery(Script script, SearchScript searchScript) {
this.script = script;
@ -158,17 +157,23 @@ public class ScriptQueryBuilder extends AbstractQueryBuilder<ScriptQueryBuilder>
@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 this == obj;
}
@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

View File

@ -20,6 +20,7 @@
package org.elasticsearch.index.query;
import org.apache.lucene.search.Query;
import org.elasticsearch.index.query.ScriptQueryBuilder.ScriptQuery;
import org.elasticsearch.script.MockScriptEngine;
import org.elasticsearch.script.Script;
import org.elasticsearch.script.ScriptService.ScriptType;
@ -41,9 +42,19 @@ public class ScriptQueryBuilderTests extends AbstractQueryTestCase<ScriptQueryBu
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 the query would not get cached
ScriptQuery sQuery = (ScriptQuery) query;
ScriptQuery clone = new ScriptQuery(sQuery.script, sQuery.searchScript);
assertFalse(sQuery.equals(clone));
assertFalse(sQuery.hashCode() == clone.hashCode());
}
public void testIllegalConstructorArg() {

View File

@ -572,6 +572,13 @@ public abstract class AbstractQueryTestCase<QB extends AbstractQueryBuilder<QB>>
return parseInnerQueryBuilder;
}
/**
* Whether the queries produced by this builder are expected to be 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.
@ -618,8 +625,10 @@ public abstract class AbstractQueryTestCase<QB extends AbstractQueryBuilder<QB>>
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());