From 36360358b2187c9d20a9495ef59ab378b6c1d765 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Thu, 27 Jun 2019 14:33:28 +0200 Subject: [PATCH] Move query builder caching check to dedicated tests (#43238) Currently `AbstractQueryTestCase#testToQuery` checks the search context cachable flag. This is a bit fragile due to the high randomization of query builders performed by this general test. Also we might only rarely check the "interesting" cases because they rarely get generated when fully randomizing the query builder. This change moved the general checks out ot #testToQuery and instead adds dedicated cache tests for those query builders that exhibit something other than the default behaviour. Closes #43200 --- .../PercolateQueryBuilderTests.java | 12 +++++-- .../ScriptScoreQueryBuilder.java | 8 ++--- .../query/MoreLikeThisQueryBuilderTests.java | 29 +++++++++++++-- .../index/query/RangeQueryBuilderTests.java | 21 +++++++++++ .../index/query/ScriptQueryBuilderTests.java | 11 ++++-- .../query/ScriptScoreQueryBuilderTests.java | 11 ++++-- .../index/query/TermsQueryBuilderTests.java | 7 ---- .../query/TermsSetQueryBuilderTests.java | 36 +++++++++++++++++-- .../index/query/TypeQueryBuilderTests.java | 6 ++++ .../FunctionScoreQueryBuilderTests.java | 36 +++++++++++++++++-- .../test/AbstractQueryTestCase.java | 24 +++++++------ 11 files changed, 167 insertions(+), 34 deletions(-) diff --git a/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolateQueryBuilderTests.java b/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolateQueryBuilderTests.java index 5b4dc610900..a86f93ce405 100644 --- a/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolateQueryBuilderTests.java +++ b/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolateQueryBuilderTests.java @@ -296,9 +296,17 @@ public class PercolateQueryBuilderTests extends AbstractQueryTestCase functionBuilder = new ScriptScoreFunctionBuilder( new Script(ScriptType.INLINE, MockScriptEngine.NAME, "1", Collections.emptyMap())); FunctionScoreQueryBuilder builder = functionScoreQuery(queryBuilder, functionBuilder); @@ -796,8 +798,38 @@ public class FunctionScoreQueryBuilderTests extends AbstractQueryTestCase scriptScoreFunction = new ScriptScoreFunctionBuilder( + new Script(ScriptType.INLINE, MockScriptEngine.NAME, "1", Collections.emptyMap())); + RandomScoreFunctionBuilder randomScoreFunctionBuilder = new RandomScoreFunctionBuilderWithFixedSeed(); + + for (ScoreFunctionBuilder scoreFunction : Arrays.asList(scriptScoreFunction, randomScoreFunctionBuilder)) { + FilterFunctionBuilder[] functions = new FilterFunctionBuilder[] { + new FilterFunctionBuilder(RandomQueryBuilder.createQuery(random()), scoreFunction) }; + queryBuilder = new FunctionScoreQueryBuilder(functions); + + context = createShardContext(); + rewriteQuery = rewriteQuery(queryBuilder, new QueryShardContext(context)); + assertNotNull(rewriteQuery.toQuery(context)); + assertFalse("query should not be cacheable: " + queryBuilder.toString(), context.isCacheable()); + } + } + + private boolean isCacheable(FunctionScoreQueryBuilder queryBuilder) { FilterFunctionBuilder[] filterFunctionBuilders = queryBuilder.filterFunctionBuilders(); for (FilterFunctionBuilder builder : filterFunctionBuilders) { if (builder.getScoreFunction() instanceof ScriptScoreFunctionBuilder) { diff --git a/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java index 3b03f95f28f..089423770d5 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java @@ -428,13 +428,6 @@ public abstract class AbstractQueryTestCase> * we first rewrite the query with a private context, then reset the context and then build the actual lucene query*/ QueryBuilder rewritten = rewriteQuery(firstQuery, new QueryShardContext(context)); Query firstLuceneQuery = rewritten.toQuery(context); - if (isCacheable(firstQuery)) { - assertTrue("query was marked as not cacheable in the context but this test indicates it should be cacheable: " - + firstQuery.toString(), context.isCacheable()); - } else { - assertFalse("query was marked as cacheable in the context but this test indicates it should not be cacheable: " - + firstQuery.toString(), context.isCacheable()); - } assertNotNull("toQuery should not return null", firstLuceneQuery); assertLuceneQuery(firstQuery, firstLuceneQuery, searchContext); //remove after assertLuceneQuery since the assertLuceneQuery impl might access the context as well @@ -478,10 +471,6 @@ public abstract class AbstractQueryTestCase> return rewritten; } - protected boolean isCacheable(QB queryBuilder) { - return true; - } - /** * Few queries allow you to set the boost on the Java API, although the corresponding parser * doesn't parse it as it isn't supported. This method allows to disable boost related tests for those queries. @@ -809,4 +798,17 @@ public abstract class AbstractQueryTestCase> public boolean isTextField(String fieldName) { return fieldName.equals(STRING_FIELD_NAME) || fieldName.equals(STRING_ALIAS_FIELD_NAME); } + + /** + * Check that a query is generally cacheable. Tests for query builders that are not always cacheable + * should overwrite this method and make sure the different cases are always tested + */ + public void testCacheability() throws IOException { + QB queryBuilder = createTestQueryBuilder(); + QueryShardContext context = createShardContext(); + QueryBuilder rewriteQuery = rewriteQuery(queryBuilder, new QueryShardContext(context)); + assertNotNull(rewriteQuery.toQuery(context)); + assertTrue("query should be cacheable: " + queryBuilder.toString(), context.isCacheable()); + } + }