From 8353d7ff73f19fd3ff7e98556da4915ee7669cd5 Mon Sep 17 00:00:00 2001 From: Michael Gibney Date: Thu, 10 Feb 2022 16:54:48 -0500 Subject: [PATCH 1/7] don't double-cache FilterQuery --- .../apache/solr/search/SolrIndexSearcher.java | 17 +++ .../solr/search/TestFiltersQueryCaching.java | 132 ++++++++++++++++++ 2 files changed, 149 insertions(+) create mode 100644 solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java diff --git a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java index 2e6e3368353..e620ea187ce 100644 --- a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java +++ b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java @@ -70,6 +70,7 @@ import org.apache.solr.metrics.MetricsMap; import org.apache.solr.metrics.SolrMetricManager; import org.apache.solr.metrics.SolrMetricsContext; +import org.apache.solr.query.FilterQuery; import org.apache.solr.request.LocalSolrQueryRequest; import org.apache.solr.request.SolrQueryRequest; import org.apache.solr.request.SolrRequestInfo; @@ -815,6 +816,9 @@ DocSet getPositiveDocSet(Query query) throws IOException { } if (query instanceof WrappedQuery) { query = ((WrappedQuery) query).getWrappedQuery(); + } else if (query instanceof FilterQuery) { + doCache = false; + // Paradoxically, this _is_ what we want. See related comment in `getDocSet(Query, DocSet)` } } @@ -1201,6 +1205,19 @@ public DocSet getDocSet(Query query, DocSet filter) throws IOException { } if (query instanceof WrappedQuery) { query = ((WrappedQuery) query).getWrappedQuery(); + } else if (query instanceof FilterQuery) { + doCache = false; + // Paradoxically, this _is_ what we want. The FilterQuery wrapper is designed to ensure that its + // inner query always consults the filterCache, regardless of the context in which it's called. + // FilterQuery internally calls SolrIndexSearcher.getDocSet with its _wrapped_ query, so the caching + // happens at that level, and we want _not_ to consult the filterCache with the FilterQuery wrapper + // per se. Allowing `doCache=true` here can result in double-entry in the filterCache (e.g. when + // using `fq=filter({!term f=field v=term})`, or caching separate clauses of a BooleanQuery via + // `fq={!bool should='filter($q1)' should='filter($q2)'}`). + // Other solutions, e.g., having FilterQuery internally get `getDocSet` with _itself_ (as + // opposed to its wrapped query), unwrapping the FilterQuery here, or any attempt to modify + // `.equals(...)` and `.hashCode()` to recognize FilterQuery as equivalent to its inner query, + // would result in deadlock against an async cache like CaffeineCache in its default configuration. } } diff --git a/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java b/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java new file mode 100644 index 00000000000..87d889ceea6 --- /dev/null +++ b/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java @@ -0,0 +1,132 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.search; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; + +import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.core.SolrCore; +import org.apache.solr.metrics.MetricsMap; +import org.apache.solr.metrics.SolrMetricManager; +import org.apache.solr.search.join.FiltersQParser; +import org.junit.BeforeClass; +import org.junit.Test; + +import static org.apache.solr.common.util.Utils.fromJSONString; + +/** + * Verify caching impacts of FiltersQParser and FilterQuery + */ +public class TestFiltersQueryCaching extends SolrTestCaseJ4 { + + private static final int NUM_DOCS = 20; + + @BeforeClass + public static void beforeClass() throws Exception { + initCore("solrconfig.xml", "schema_latest.xml"); + createIndex(); + } + + public static void createIndex() { + for (int i = 0; i < NUM_DOCS; i++) { + assertU(adoc("id", Integer.toString(i), "field_s", "d" + i)); + if (random().nextInt(NUM_DOCS) == 0) { + assertU(commit()); // sometimes make multiple segments + } + } + assertU(commit()); + } + + private static final String MATCH_ALL_DOCS_QUERY = "*:*"; + + private static Map coreToFilterCacheMetrics(SolrCore core) { + return ((MetricsMap)((SolrMetricManager.GaugeWrapper)core.getCoreMetricManager().getRegistry() + .getMetrics().get("CACHE.searcher.filterCache")).getGauge()).getValue(); + } + + private static long coreToInserts(SolrCore core) { + return (long)((MetricsMap)((SolrMetricManager.GaugeWrapper)core + .getCoreMetricManager().getRegistry().getMetrics().get("CACHE.searcher.filterCache")).getGauge()) + .getValue().get("inserts"); + } + + private static long getNumFound(String response) { + Map res = (Map) fromJSONString(response); + Map body = (Map) (res.get("response")); + return (long) body.get("numFound"); + } + + @Test + public void testRecursiveFilter() throws Exception { + final String termQuery = "{!term f=field_s v='d0'}"; + final String filterTermQuery = "filter(" + termQuery + ")"; + final int expectNumFound = 1; + String response; + + h.reload(); + response = JQ(req("q", termQuery, "indent", "true")); + assertEquals(0, coreToInserts(h.getCore())); + assertEquals(expectNumFound, getNumFound(response)); + + h.reload(); + response = JQ(req("q", filterTermQuery, "indent", "true")); + assertEquals(1, coreToInserts(h.getCore())); + assertEquals(expectNumFound, getNumFound(response)); + + h.reload(); + response = JQ(req("q", MATCH_ALL_DOCS_QUERY, "indent", "true", "fq", termQuery)); + assertEquals(1, coreToInserts(h.getCore())); + assertEquals(expectNumFound, getNumFound(response)); + + h.reload(); + response = JQ(req("q", MATCH_ALL_DOCS_QUERY, "indent", "true", "fq", filterTermQuery)); + assertEquals(1, coreToInserts(h.getCore())); + assertEquals(expectNumFound, getNumFound(response)); + + h.reload(); + SolrCore core = h.getCore(); + Map filterCacheMetrics; + final String termQuery2 = "{!term f=field_s v='d1'}"; + final String filterTermQuery2 = "filter(" + termQuery2 + ")"; + response = JQ(req("q", MATCH_ALL_DOCS_QUERY, "indent", "true", "fq", "{!bool cache=false should=$ftq should=$ftq2}", + "ftq", filterTermQuery, "ftq2", filterTermQuery2)); + assertEquals(2, coreToInserts(core)); + assertEquals(2, getNumFound(response)); + JQ(req("q", MATCH_ALL_DOCS_QUERY, "indent", "true", "fq", random().nextBoolean() ? termQuery : filterTermQuery)); + filterCacheMetrics = coreToFilterCacheMetrics(core); + assertEquals(2, (long) filterCacheMetrics.get("inserts")); // unchanged + assertEquals(1, (long) filterCacheMetrics.get("hits")); + JQ(req("q", MATCH_ALL_DOCS_QUERY, "indent", "true", "fq", random().nextBoolean() ? termQuery2 : filterTermQuery2)); + filterCacheMetrics = coreToFilterCacheMetrics(core); + assertEquals(2, (long) filterCacheMetrics.get("inserts")); // unchanged + assertEquals(2, (long) filterCacheMetrics.get("hits")); + JQ(req("q", MATCH_ALL_DOCS_QUERY, "indent", "true", "fq", "{!bool cache=false should=$ftq should=$ftq2}", + "ftq", filterTermQuery, "ftq2", filterTermQuery2, "cursorMark", "*", "sort", "id asc")); + filterCacheMetrics = coreToFilterCacheMetrics(core); + assertEquals(2, (long) filterCacheMetrics.get("inserts")); // unchanged + assertEquals(4, (long) filterCacheMetrics.get("hits")); + JQ(req("q", MATCH_ALL_DOCS_QUERY, "indent", "true", "fq", "{!bool should=$ftq should=$ftq2}", + "ftq", filterTermQuery, "ftq2", filterTermQuery2, "cursorMark", "*", "sort", "id asc")); + filterCacheMetrics = coreToFilterCacheMetrics(core); + assertEquals(3, (long) filterCacheMetrics.get("inserts")); // added top-level + assertEquals(6, (long) filterCacheMetrics.get("hits")); + } +} From 6095389ebc7014c56f8d8c23283d72c21509c1b3 Mon Sep 17 00:00:00 2001 From: Michael Gibney Date: Thu, 10 Feb 2022 17:39:55 -0500 Subject: [PATCH 2/7] remove unused imports --- .../test/org/apache/solr/search/TestFiltersQueryCaching.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java b/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java index 87d889ceea6..9967af37e21 100644 --- a/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java +++ b/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java @@ -16,17 +16,12 @@ */ package org.apache.solr.search; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.List; import java.util.Map; import org.apache.solr.SolrTestCaseJ4; import org.apache.solr.core.SolrCore; import org.apache.solr.metrics.MetricsMap; import org.apache.solr.metrics.SolrMetricManager; -import org.apache.solr.search.join.FiltersQParser; import org.junit.BeforeClass; import org.junit.Test; From f29144a849c5758b053ae30b8e0e95126824ec97 Mon Sep 17 00:00:00 2001 From: Michael Gibney Date: Wed, 16 Feb 2022 13:32:05 -0500 Subject: [PATCH 3/7] minor refactoring of test suite --- .../solr/search/TestFiltersQueryCaching.java | 64 ++++++++----------- 1 file changed, 27 insertions(+), 37 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java b/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java index 9967af37e21..1dd32c013b2 100644 --- a/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java +++ b/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java @@ -25,8 +25,6 @@ import org.junit.BeforeClass; import org.junit.Test; -import static org.apache.solr.common.util.Utils.fromJSONString; - /** * Verify caching impacts of FiltersQParser and FilterQuery */ @@ -50,77 +48,69 @@ public static void createIndex() { assertU(commit()); } - private static final String MATCH_ALL_DOCS_QUERY = "*:*"; - - private static Map coreToFilterCacheMetrics(SolrCore core) { + private static Map lookupFilterCacheMetrics(SolrCore core) { return ((MetricsMap)((SolrMetricManager.GaugeWrapper)core.getCoreMetricManager().getRegistry() .getMetrics().get("CACHE.searcher.filterCache")).getGauge()).getValue(); } - private static long coreToInserts(SolrCore core) { + private static long lookupFilterCacheInserts(SolrCore core) { return (long)((MetricsMap)((SolrMetricManager.GaugeWrapper)core .getCoreMetricManager().getRegistry().getMetrics().get("CACHE.searcher.filterCache")).getGauge()) .getValue().get("inserts"); } - private static long getNumFound(String response) { - Map res = (Map) fromJSONString(response); - Map body = (Map) (res.get("response")); - return (long) body.get("numFound"); - } - @Test public void testRecursiveFilter() throws Exception { final String termQuery = "{!term f=field_s v='d0'}"; final String filterTermQuery = "filter(" + termQuery + ")"; final int expectNumFound = 1; - String response; + final String expectNumFoundXPath = "/response/numFound==" + expectNumFound; h.reload(); - response = JQ(req("q", termQuery, "indent", "true")); - assertEquals(0, coreToInserts(h.getCore())); - assertEquals(expectNumFound, getNumFound(response)); + assertJQ(req("q", termQuery, "indent", "true"), + expectNumFoundXPath); + assertEquals(0, lookupFilterCacheInserts(h.getCore())); h.reload(); - response = JQ(req("q", filterTermQuery, "indent", "true")); - assertEquals(1, coreToInserts(h.getCore())); - assertEquals(expectNumFound, getNumFound(response)); + assertJQ(req("q", filterTermQuery, "indent", "true"), + expectNumFoundXPath); + assertEquals(1, lookupFilterCacheInserts(h.getCore())); h.reload(); - response = JQ(req("q", MATCH_ALL_DOCS_QUERY, "indent", "true", "fq", termQuery)); - assertEquals(1, coreToInserts(h.getCore())); - assertEquals(expectNumFound, getNumFound(response)); + assertJQ(req("q", "*:*", "indent", "true", "fq", termQuery), + expectNumFoundXPath); + assertEquals(1, lookupFilterCacheInserts(h.getCore())); h.reload(); - response = JQ(req("q", MATCH_ALL_DOCS_QUERY, "indent", "true", "fq", filterTermQuery)); - assertEquals(1, coreToInserts(h.getCore())); - assertEquals(expectNumFound, getNumFound(response)); + assertJQ(req("q", "*:*", "indent", "true", "fq", filterTermQuery), + expectNumFoundXPath); + assertEquals(1, lookupFilterCacheInserts(h.getCore())); h.reload(); SolrCore core = h.getCore(); Map filterCacheMetrics; final String termQuery2 = "{!term f=field_s v='d1'}"; final String filterTermQuery2 = "filter(" + termQuery2 + ")"; - response = JQ(req("q", MATCH_ALL_DOCS_QUERY, "indent", "true", "fq", "{!bool cache=false should=$ftq should=$ftq2}", - "ftq", filterTermQuery, "ftq2", filterTermQuery2)); - assertEquals(2, coreToInserts(core)); - assertEquals(2, getNumFound(response)); - JQ(req("q", MATCH_ALL_DOCS_QUERY, "indent", "true", "fq", random().nextBoolean() ? termQuery : filterTermQuery)); - filterCacheMetrics = coreToFilterCacheMetrics(core); + assertJQ(req("q", "*:*", "indent", "true", "fq", "{!bool cache=false should=$ftq should=$ftq2}", + "ftq", filterTermQuery, "ftq2", filterTermQuery2), + "/response/numFound==2"); + assertEquals(2, lookupFilterCacheInserts(core)); + JQ(req("q", "*:*", "indent", "true", "fq", random().nextBoolean() ? termQuery : filterTermQuery)); + filterCacheMetrics = lookupFilterCacheMetrics(core); assertEquals(2, (long) filterCacheMetrics.get("inserts")); // unchanged assertEquals(1, (long) filterCacheMetrics.get("hits")); - JQ(req("q", MATCH_ALL_DOCS_QUERY, "indent", "true", "fq", random().nextBoolean() ? termQuery2 : filterTermQuery2)); - filterCacheMetrics = coreToFilterCacheMetrics(core); + JQ(req("q", "*:*", "indent", "true", "fq", random().nextBoolean() ? termQuery2 : filterTermQuery2)); + filterCacheMetrics = lookupFilterCacheMetrics(core); assertEquals(2, (long) filterCacheMetrics.get("inserts")); // unchanged assertEquals(2, (long) filterCacheMetrics.get("hits")); - JQ(req("q", MATCH_ALL_DOCS_QUERY, "indent", "true", "fq", "{!bool cache=false should=$ftq should=$ftq2}", + JQ(req("q", "*:*", "indent", "true", "fq", "{!bool cache=false should=$ftq should=$ftq2}", "ftq", filterTermQuery, "ftq2", filterTermQuery2, "cursorMark", "*", "sort", "id asc")); - filterCacheMetrics = coreToFilterCacheMetrics(core); + filterCacheMetrics = lookupFilterCacheMetrics(core); assertEquals(2, (long) filterCacheMetrics.get("inserts")); // unchanged assertEquals(4, (long) filterCacheMetrics.get("hits")); - JQ(req("q", MATCH_ALL_DOCS_QUERY, "indent", "true", "fq", "{!bool should=$ftq should=$ftq2}", + JQ(req("q", "*:*", "indent", "true", "fq", "{!bool should=$ftq should=$ftq2}", "ftq", filterTermQuery, "ftq2", filterTermQuery2, "cursorMark", "*", "sort", "id asc")); - filterCacheMetrics = coreToFilterCacheMetrics(core); + filterCacheMetrics = lookupFilterCacheMetrics(core); assertEquals(3, (long) filterCacheMetrics.get("inserts")); // added top-level assertEquals(6, (long) filterCacheMetrics.get("hits")); } From 2df8cf0b87916d829097097bc6e88fa6db5a4113 Mon Sep 17 00:00:00 2001 From: Michael Gibney Date: Fri, 18 Feb 2022 13:37:12 -0500 Subject: [PATCH 4/7] override FilterQuery.get/setCache() instead of special-casing in SolrIndexSearcher --- .../org/apache/solr/query/FilterQuery.java | 21 +++++++++++++++++++ .../apache/solr/search/SolrIndexSearcher.java | 17 --------------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/query/FilterQuery.java b/solr/core/src/java/org/apache/solr/query/FilterQuery.java index 7938c2a71bb..744fb2b2370 100644 --- a/solr/core/src/java/org/apache/solr/query/FilterQuery.java +++ b/solr/core/src/java/org/apache/solr/query/FilterQuery.java @@ -41,6 +41,27 @@ public FilterQuery(Query q) { this.q = q; } + @Override + public final void setCache(boolean cache) { + if (cache) { + // because of the special syntax for invoking FilterQuery (i.e., `filter([backing_query])` -- different from + // the normal QParser + localParams syntax) there should be no risk of attempting to set `cache=true`. + throw new IllegalArgumentException(FilterQuery.class + " caches internally and does not support external setCache(true)"); + } + } + + @Override + public final boolean getCache() { + return false; + // Paradoxically, this _is_ what we want. The FilterQuery wrapper is designed to ensure that its + // inner query always consults the filterCache, regardless of the context in which it's called. + // FilterQuery internally calls SolrIndexSearcher.getDocSet with its _wrapped_ query, so the caching + // happens at that level, and we want _not_ to consult the filterCache with the FilterQuery wrapper + // per se. Allowing `doCache=true` here can result in double-entry in the filterCache (e.g. when + // using `fq=filter({!term f=field v=term})`, or caching separate clauses of a BooleanQuery via + // `fq={!bool should='filter($q1)' should='filter($q2)'}`). + } + public Query getQuery() { return q; } diff --git a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java index c31d4714fc4..c909093bb79 100644 --- a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java +++ b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java @@ -70,7 +70,6 @@ import org.apache.solr.metrics.MetricsMap; import org.apache.solr.metrics.SolrMetricManager; import org.apache.solr.metrics.SolrMetricsContext; -import org.apache.solr.query.FilterQuery; import org.apache.solr.request.LocalSolrQueryRequest; import org.apache.solr.request.SolrQueryRequest; import org.apache.solr.request.SolrRequestInfo; @@ -816,9 +815,6 @@ DocSet getPositiveDocSet(Query query) throws IOException { } if (query instanceof WrappedQuery) { query = ((WrappedQuery) query).getWrappedQuery(); - } else if (query instanceof FilterQuery) { - doCache = false; - // Paradoxically, this _is_ what we want. See related comment in `getDocSet(Query, DocSet)` } } @@ -1205,19 +1201,6 @@ public DocSet getDocSet(Query query, DocSet filter) throws IOException { } if (query instanceof WrappedQuery) { query = ((WrappedQuery) query).getWrappedQuery(); - } else if (query instanceof FilterQuery) { - doCache = false; - // Paradoxically, this _is_ what we want. The FilterQuery wrapper is designed to ensure that its - // inner query always consults the filterCache, regardless of the context in which it's called. - // FilterQuery internally calls SolrIndexSearcher.getDocSet with its _wrapped_ query, so the caching - // happens at that level, and we want _not_ to consult the filterCache with the FilterQuery wrapper - // per se. Allowing `doCache=true` here can result in double-entry in the filterCache (e.g. when - // using `fq=filter({!term f=field v=term})`, or caching separate clauses of a BooleanQuery via - // `fq={!bool should='filter($q1)' should='filter($q2)'}`). - // Other solutions, e.g., having FilterQuery internally get `getDocSet` with _itself_ (as - // opposed to its wrapped query), unwrapping the FilterQuery here, or any attempt to modify - // `.equals(...)` and `.hashCode()` to recognize FilterQuery as equivalent to its inner query, - // would result in deadlock against an async cache like CaffeineCache in its default configuration. } } From 1b4d7d1a380ae420ea99328929e8b7b52e9a0e33 Mon Sep 17 00:00:00 2001 From: Michael Gibney Date: Tue, 22 Feb 2022 12:49:10 -0500 Subject: [PATCH 5/7] clarify semantics of `{!cache=false|true}filter(q)` --- .../org/apache/solr/query/FilterQuery.java | 31 ++++++++++++------- .../solr/search/TestFiltersQueryCaching.java | 20 ++++++++++++ 2 files changed, 39 insertions(+), 12 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/query/FilterQuery.java b/solr/core/src/java/org/apache/solr/query/FilterQuery.java index 744fb2b2370..6bef6e53198 100644 --- a/solr/core/src/java/org/apache/solr/query/FilterQuery.java +++ b/solr/core/src/java/org/apache/solr/query/FilterQuery.java @@ -43,23 +43,30 @@ public FilterQuery(Query q) { @Override public final void setCache(boolean cache) { - if (cache) { - // because of the special syntax for invoking FilterQuery (i.e., `filter([backing_query])` -- different from - // the normal QParser + localParams syntax) there should be no risk of attempting to set `cache=true`. - throw new IllegalArgumentException(FilterQuery.class + " caches internally and does not support external setCache(true)"); - } + /* + NOTE: at the implementation level, we silently ignore explicit `setCache` directives. But at a higher level + (i.e., from the client's perspective) by ignoring at the implementation level, we in fact respect the semantics + both of explicit `{!cache=false}filter(q)` and `{!cache=true}filter(q)`. Since the purpose of FilterQuery + is to handle caching _internal_ to the query, external `cache=false` _should_ have no effect. Slightly + less intuitive: the `cache=true` case should be interpreted as directing "ensure that we consult the + filterCache for this query" -- and indeed because caching is handled internally, the essence of the + top-level `cache=true` directive is most appropriately respected by having `getCache()` continue to return + `false` at the level of the FilterQuery _per se_. + */ } @Override public final boolean getCache() { return false; - // Paradoxically, this _is_ what we want. The FilterQuery wrapper is designed to ensure that its - // inner query always consults the filterCache, regardless of the context in which it's called. - // FilterQuery internally calls SolrIndexSearcher.getDocSet with its _wrapped_ query, so the caching - // happens at that level, and we want _not_ to consult the filterCache with the FilterQuery wrapper - // per se. Allowing `doCache=true` here can result in double-entry in the filterCache (e.g. when - // using `fq=filter({!term f=field v=term})`, or caching separate clauses of a BooleanQuery via - // `fq={!bool should='filter($q1)' should='filter($q2)'}`). + /* + Paradoxically, this _is_ what we want. The FilterQuery wrapper is designed to ensure that its + inner query always consults the filterCache, regardless of the context in which it's called. + FilterQuery internally calls SolrIndexSearcher.getDocSet with its _wrapped_ query, so the caching + happens at that level, and we want _not_ to consult the filterCache with the FilterQuery wrapper + per se. Allowing `getCache()=true` here can result in double-entry in the filterCache (e.g. when + using `fq=filter({!term f=field v=term})`, or caching separate clauses of a BooleanQuery via + `fq={!bool should='filter($q1)' should='filter($q2)'}`). + */ } public Query getQuery() { diff --git a/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java b/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java index 1dd32c013b2..886653f4d23 100644 --- a/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java +++ b/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java @@ -76,6 +76,26 @@ public void testRecursiveFilter() throws Exception { expectNumFoundXPath); assertEquals(1, lookupFilterCacheInserts(h.getCore())); + h.reload(); + assertJQ(req("q", "*:*", "indent", "true", "fq", "{!cache=false}field_s:d0"), + expectNumFoundXPath); + assertEquals(0, lookupFilterCacheInserts(h.getCore())); + + h.reload(); + assertJQ(req("q", "*:*", "indent", "true", "fq", "{!cache=true}field_s:d0"), + expectNumFoundXPath); + assertEquals(1, lookupFilterCacheInserts(h.getCore())); + + h.reload(); + assertJQ(req("q", "*:*", "indent", "true", "fq", "{!cache=false}filter(field_s:d0)"), + expectNumFoundXPath); + assertEquals(1, lookupFilterCacheInserts(h.getCore())); + + h.reload(); + assertJQ(req("q", "*:*", "indent", "true", "fq", "{!cache=true}filter(field_s:d0)"), + expectNumFoundXPath); + assertEquals(1, lookupFilterCacheInserts(h.getCore())); + h.reload(); assertJQ(req("q", "*:*", "indent", "true", "fq", termQuery), expectNumFoundXPath); From b957a1e898be97a6c18ba40aa27868e0bd951f2d Mon Sep 17 00:00:00 2001 From: Michael Gibney Date: Tue, 29 Mar 2022 10:48:47 -0400 Subject: [PATCH 6/7] add CHANGES.txt entry --- solr/CHANGES.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index aae1da49476..22c0497a394 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -39,6 +39,8 @@ Improvements * SOLR-15652: Add Slack channel for community support to Admin UI footer, update links. (Eric Pugh) +* SOLR-16002: Avoid redundant `FilterQuery` caching, e.g. via `filter($query)` syntax. (Michael Gibney, David Smiley) + Optimizations --------------------- (No changes) From da7ef23bc8ce1f6a85670f429e701bc3f7bf51f0 Mon Sep 17 00:00:00 2001 From: Michael Gibney Date: Tue, 29 Mar 2022 13:07:51 -0400 Subject: [PATCH 7/7] address spotless formatting errors --- .../solr/search/TestFiltersQueryCaching.java | 130 +++++++++++++----- 1 file changed, 95 insertions(+), 35 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java b/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java index 886653f4d23..cf797fef425 100644 --- a/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java +++ b/solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java @@ -17,7 +17,6 @@ package org.apache.solr.search; import java.util.Map; - import org.apache.solr.SolrTestCaseJ4; import org.apache.solr.core.SolrCore; import org.apache.solr.metrics.MetricsMap; @@ -25,9 +24,7 @@ import org.junit.BeforeClass; import org.junit.Test; -/** - * Verify caching impacts of FiltersQParser and FilterQuery - */ +/** Verify caching impacts of FiltersQParser and FilterQuery */ public class TestFiltersQueryCaching extends SolrTestCaseJ4 { private static final int NUM_DOCS = 20; @@ -42,21 +39,34 @@ public static void createIndex() { for (int i = 0; i < NUM_DOCS; i++) { assertU(adoc("id", Integer.toString(i), "field_s", "d" + i)); if (random().nextInt(NUM_DOCS) == 0) { - assertU(commit()); // sometimes make multiple segments + assertU(commit()); // sometimes make multiple segments } } assertU(commit()); } private static Map lookupFilterCacheMetrics(SolrCore core) { - return ((MetricsMap)((SolrMetricManager.GaugeWrapper)core.getCoreMetricManager().getRegistry() - .getMetrics().get("CACHE.searcher.filterCache")).getGauge()).getValue(); + return ((MetricsMap) + ((SolrMetricManager.GaugeWrapper) + core.getCoreMetricManager() + .getRegistry() + .getMetrics() + .get("CACHE.searcher.filterCache")) + .getGauge()) + .getValue(); } private static long lookupFilterCacheInserts(SolrCore core) { - return (long)((MetricsMap)((SolrMetricManager.GaugeWrapper)core - .getCoreMetricManager().getRegistry().getMetrics().get("CACHE.searcher.filterCache")).getGauge()) - .getValue().get("inserts"); + return (long) + ((MetricsMap) + ((SolrMetricManager.GaugeWrapper) + core.getCoreMetricManager() + .getRegistry() + .getMetrics() + .get("CACHE.searcher.filterCache")) + .getGauge()) + .getValue() + .get("inserts"); } @Test @@ -67,43 +77,41 @@ public void testRecursiveFilter() throws Exception { final String expectNumFoundXPath = "/response/numFound==" + expectNumFound; h.reload(); - assertJQ(req("q", termQuery, "indent", "true"), - expectNumFoundXPath); + assertJQ(req("q", termQuery, "indent", "true"), expectNumFoundXPath); assertEquals(0, lookupFilterCacheInserts(h.getCore())); h.reload(); - assertJQ(req("q", filterTermQuery, "indent", "true"), - expectNumFoundXPath); + assertJQ(req("q", filterTermQuery, "indent", "true"), expectNumFoundXPath); assertEquals(1, lookupFilterCacheInserts(h.getCore())); h.reload(); - assertJQ(req("q", "*:*", "indent", "true", "fq", "{!cache=false}field_s:d0"), - expectNumFoundXPath); + assertJQ( + req("q", "*:*", "indent", "true", "fq", "{!cache=false}field_s:d0"), expectNumFoundXPath); assertEquals(0, lookupFilterCacheInserts(h.getCore())); h.reload(); - assertJQ(req("q", "*:*", "indent", "true", "fq", "{!cache=true}field_s:d0"), - expectNumFoundXPath); + assertJQ( + req("q", "*:*", "indent", "true", "fq", "{!cache=true}field_s:d0"), expectNumFoundXPath); assertEquals(1, lookupFilterCacheInserts(h.getCore())); h.reload(); - assertJQ(req("q", "*:*", "indent", "true", "fq", "{!cache=false}filter(field_s:d0)"), - expectNumFoundXPath); + assertJQ( + req("q", "*:*", "indent", "true", "fq", "{!cache=false}filter(field_s:d0)"), + expectNumFoundXPath); assertEquals(1, lookupFilterCacheInserts(h.getCore())); h.reload(); - assertJQ(req("q", "*:*", "indent", "true", "fq", "{!cache=true}filter(field_s:d0)"), - expectNumFoundXPath); + assertJQ( + req("q", "*:*", "indent", "true", "fq", "{!cache=true}filter(field_s:d0)"), + expectNumFoundXPath); assertEquals(1, lookupFilterCacheInserts(h.getCore())); h.reload(); - assertJQ(req("q", "*:*", "indent", "true", "fq", termQuery), - expectNumFoundXPath); + assertJQ(req("q", "*:*", "indent", "true", "fq", termQuery), expectNumFoundXPath); assertEquals(1, lookupFilterCacheInserts(h.getCore())); h.reload(); - assertJQ(req("q", "*:*", "indent", "true", "fq", filterTermQuery), - expectNumFoundXPath); + assertJQ(req("q", "*:*", "indent", "true", "fq", filterTermQuery), expectNumFoundXPath); assertEquals(1, lookupFilterCacheInserts(h.getCore())); h.reload(); @@ -111,25 +119,77 @@ public void testRecursiveFilter() throws Exception { Map filterCacheMetrics; final String termQuery2 = "{!term f=field_s v='d1'}"; final String filterTermQuery2 = "filter(" + termQuery2 + ")"; - assertJQ(req("q", "*:*", "indent", "true", "fq", "{!bool cache=false should=$ftq should=$ftq2}", - "ftq", filterTermQuery, "ftq2", filterTermQuery2), - "/response/numFound==2"); + assertJQ( + req( + "q", + "*:*", + "indent", + "true", + "fq", + "{!bool cache=false should=$ftq should=$ftq2}", + "ftq", + filterTermQuery, + "ftq2", + filterTermQuery2), + "/response/numFound==2"); assertEquals(2, lookupFilterCacheInserts(core)); - JQ(req("q", "*:*", "indent", "true", "fq", random().nextBoolean() ? termQuery : filterTermQuery)); + JQ( + req( + "q", + "*:*", + "indent", + "true", + "fq", + random().nextBoolean() ? termQuery : filterTermQuery)); filterCacheMetrics = lookupFilterCacheMetrics(core); assertEquals(2, (long) filterCacheMetrics.get("inserts")); // unchanged assertEquals(1, (long) filterCacheMetrics.get("hits")); - JQ(req("q", "*:*", "indent", "true", "fq", random().nextBoolean() ? termQuery2 : filterTermQuery2)); + JQ( + req( + "q", + "*:*", + "indent", + "true", + "fq", + random().nextBoolean() ? termQuery2 : filterTermQuery2)); filterCacheMetrics = lookupFilterCacheMetrics(core); assertEquals(2, (long) filterCacheMetrics.get("inserts")); // unchanged assertEquals(2, (long) filterCacheMetrics.get("hits")); - JQ(req("q", "*:*", "indent", "true", "fq", "{!bool cache=false should=$ftq should=$ftq2}", - "ftq", filterTermQuery, "ftq2", filterTermQuery2, "cursorMark", "*", "sort", "id asc")); + JQ( + req( + "q", + "*:*", + "indent", + "true", + "fq", + "{!bool cache=false should=$ftq should=$ftq2}", + "ftq", + filterTermQuery, + "ftq2", + filterTermQuery2, + "cursorMark", + "*", + "sort", + "id asc")); filterCacheMetrics = lookupFilterCacheMetrics(core); assertEquals(2, (long) filterCacheMetrics.get("inserts")); // unchanged assertEquals(4, (long) filterCacheMetrics.get("hits")); - JQ(req("q", "*:*", "indent", "true", "fq", "{!bool should=$ftq should=$ftq2}", - "ftq", filterTermQuery, "ftq2", filterTermQuery2, "cursorMark", "*", "sort", "id asc")); + JQ( + req( + "q", + "*:*", + "indent", + "true", + "fq", + "{!bool should=$ftq should=$ftq2}", + "ftq", + filterTermQuery, + "ftq2", + filterTermQuery2, + "cursorMark", + "*", + "sort", + "id asc")); filterCacheMetrics = lookupFilterCacheMetrics(core); assertEquals(3, (long) filterCacheMetrics.get("inserts")); // added top-level assertEquals(6, (long) filterCacheMetrics.get("hits"));