SOLR-16002: don't double-cache FilterQuery

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;
   }

solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java

@@ -0,0 +1,117 @@
+/*
+ * 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.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.junit.BeforeClass;
+import org.junit.Test;
+
+/**
+ * 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 Map<String, Object> lookupFilterCacheMetrics(SolrCore core) {
+    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");
+  }
+
+  @Test
+  public void testRecursiveFilter() throws Exception {
+    final String termQuery = "{!term f=field_s v='d0'}";
+    final String filterTermQuery = "filter(" + termQuery + ")";
+    final int expectNumFound = 1;
+    final String expectNumFoundXPath = "/response/numFound==" + expectNumFound;
+
+    h.reload();
+    assertJQ(req("q", termQuery, "indent", "true"),
+            expectNumFoundXPath);
+    assertEquals(0, lookupFilterCacheInserts(h.getCore()));
+
+    h.reload();
+    assertJQ(req("q", filterTermQuery, "indent", "true"),
+            expectNumFoundXPath);
+    assertEquals(1, lookupFilterCacheInserts(h.getCore()));
+
+    h.reload();
+    assertJQ(req("q", "*:*", "indent", "true", "fq", termQuery),
+            expectNumFoundXPath);
+    assertEquals(1, lookupFilterCacheInserts(h.getCore()));
+
+    h.reload();
+    assertJQ(req("q", "*:*", "indent", "true", "fq", filterTermQuery),
+            expectNumFoundXPath);
+    assertEquals(1, lookupFilterCacheInserts(h.getCore()));
+
+    h.reload();
+    SolrCore core = h.getCore();
+    Map<String, Object> 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");
+    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", "*:*", "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"));
+    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"));
+    filterCacheMetrics = lookupFilterCacheMetrics(core);
+    assertEquals(3, (long) filterCacheMetrics.get("inserts")); // added top-level
+    assertEquals(6, (long) filterCacheMetrics.get("hits"));
+  }
+}