Disable request cache for queries on fields with non-default use_similarity or split_queries_on_whitespace

CHANGELOG.md

@@ -61,6 +61,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - [Flaky Test] Fix flaky test in SecureReactorNetty4HttpServerTransportTests with reproducible seed ([#19327](https://github.com/opensearch-project/OpenSearch/pull/19327))
 - Remove unnecessary looping in field data cache clear ([#19116](https://github.com/opensearch-project/OpenSearch/pull/19116))
 - [Flaky Test] Fix flaky test IngestFromKinesisIT.testAllActiveIngestion ([#19380](https://github.com/opensearch-project/OpenSearch/pull/19380))
+- Fix request cache entries not being wiped by mapping updates ([#19385](https://github.com/opensearch-project/OpenSearch/pull/19385))
 
 ### Dependencies
 - Bump `com.netflix.nebula.ospackage-base` from 12.0.0 to 12.1.0 ([#19019](https://github.com/opensearch-project/OpenSearch/pull/19019))

server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheIT.java

@@ -47,6 +47,7 @@
 import org.opensearch.action.admin.indices.alias.Alias;
 import org.opensearch.action.admin.indices.cache.clear.ClearIndicesCacheRequest;
 import org.opensearch.action.admin.indices.forcemerge.ForceMergeResponse;
+import org.opensearch.action.admin.indices.mapping.put.PutMappingRequest;
 import org.opensearch.action.search.SearchResponse;
 import org.opensearch.action.search.SearchType;
 import org.opensearch.cluster.ClusterState;
@@ -65,6 +66,7 @@
 import org.opensearch.index.query.QueryBuilders;
 import org.opensearch.index.query.QueryShardContext;
 import org.opensearch.index.query.TermQueryBuilder;
+import org.opensearch.search.SearchHit;
 import org.opensearch.search.aggregations.bucket.global.GlobalAggregationBuilder;
 import org.opensearch.search.aggregations.bucket.histogram.DateHistogramInterval;
 import org.opensearch.search.aggregations.bucket.histogram.Histogram;
@@ -861,6 +863,80 @@ public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float bo
         assertEquals(0, requestCacheStats.getMemorySizeInBytes());
     }
 
+    public void testMappingUpdateClearsCache() throws Exception {
+        String node = internalCluster().startNode(
+            Settings.builder().put(IndicesRequestCache.INDICES_REQUEST_CACHE_CLEANUP_INTERVAL_SETTING_KEY, TimeValue.timeValueMillis(1))
+        ); // Set IRC cleanup frequency low to ensure we don't get false positives when we check the cache hasn't been cleared
+        Client client = client(node);
+        String index = "index";
+        String field = "k";
+        assertAcked(
+            client.admin()
+                .indices()
+                .prepareCreate(index)
+                .setMapping("k", "type=keyword,use_similarity=false")
+                .setSettings(
+                    Settings.builder()
+                        .put(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING.getKey(), true)
+                        .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
+                        .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0)
+                        // Disable index refreshing to avoid cache being invalidated mid-test
+                        .put(IndexSettings.INDEX_REFRESH_INTERVAL_SETTING.getKey(), TimeValue.timeValueMillis(-1))
+                )
+                .get()
+        );
+        for (int i = 0; i < 5; i++) {
+            indexRandom(true, client.prepareIndex(index).setSource(field, "foo"));
+        }
+        indexRandom(true, client.prepareIndex(index).setSource(field, "bar"));
+        ensureSearchable(index);
+        // Force merge the index to ensure there can be no background merges during the subsequent searches that would invalidate the cache
+        forceMerge(client, index);
+
+        SearchResponse resp = client.prepareSearch(index)
+            .setRequestCache(true)
+            .setQuery(QueryBuilders.disMaxQuery().add(QueryBuilders.termQuery(field, "foo")).add(QueryBuilders.termQuery(field, "bar")))
+            .get();
+        assertSearchResponse(resp);
+        // When use_similarity=false all 6 docs should have equal scores
+        double delta = 0.0001;
+        float score = resp.getHits().getMaxScore();
+        for (SearchHit hit : resp.getHits()) {
+            assertEquals(score, hit.getScore(), delta);
+        }
+        assertEquals(resp.getHits().getHits().length, 6);
+
+        RequestCacheStats stats = getNodeCacheStats(client);
+        assertTrue(stats.getMemorySizeInBytes() > 0);
+
+        PutMappingRequest mappingRequest = new PutMappingRequest().indices(index).source(field, "type=keyword,use_similarity=true");
+        client.admin().indices().putMapping(mappingRequest).actionGet();
+
+        // The request cache should be wiped
+        assertBusy(() -> { assertEquals(getNodeCacheStats(client).getMemorySizeInBytes(), 0); });
+
+        // Run same search again and ensure docs do NOT all have same scores anymore, and that RC is not used (no hits)
+        resp = client.prepareSearch(index)
+            .setRequestCache(true)
+            .setQuery(QueryBuilders.disMaxQuery().add(QueryBuilders.termQuery(field, "foo")).add(QueryBuilders.termQuery(field, "bar")))
+            .get();
+        assertSearchResponse(resp);
+        // the doc with the rarer term should now have a higher score than the others, which should all have equal scores
+        assertEquals(resp.getHits().getHits().length, 6);
+        float maxScore = resp.getHits().getMaxScore();
+        assertEquals(resp.getHits().getAt(0).getScore(), maxScore, delta);
+        float otherScore = resp.getHits().getAt(1).getScore();
+        assertTrue(maxScore > otherScore);
+        for (int i = 2; i < 6; i++) {
+            assertEquals(resp.getHits().getAt(i).getScore(), otherScore, delta);
+        }
+
+        // There should be no request cache hits
+        stats = getNodeCacheStats(client);
+        assertEquals(0, stats.getHitCount());
+        assertTrue(stats.getMemorySizeInBytes() > 0);
+    }
+
     private Path[] shardDirectory(String server, Index index, int shard) {
         NodeEnvironment env = internalCluster().getInstance(NodeEnvironment.class, server);
         final Path[] paths = env.availableShardPaths(new ShardId(index, shard));

server/src/main/java/org/opensearch/cluster/ClusterChangedEvent.java

@@ -35,6 +35,7 @@
 import org.opensearch.cluster.metadata.IndexGraveyard;
 import org.opensearch.cluster.metadata.IndexGraveyard.IndexGraveyardDiff;
 import org.opensearch.cluster.metadata.IndexMetadata;
+import org.opensearch.cluster.metadata.MappingMetadata;
 import org.opensearch.cluster.metadata.Metadata;
 import org.opensearch.cluster.node.DiscoveryNodes;
 import org.opensearch.common.annotation.PublicApi;
@@ -201,6 +202,15 @@ public static boolean indexMetadataChanged(IndexMetadata metadata1, IndexMetadat
         return metadata1 != metadata2;
     }
 
+    /**
+     * Returns <code>true</code> iff the {@link MappingMetadata} for a given index
+     * has changed between the previous cluster state and the new cluster state.
+     */
+    public static boolean indexMappingMetadataChanged(IndexMetadata metadata1, IndexMetadata metadata2) {
+        assert metadata1 != null && metadata2 != null;
+        return metadata1.mapping() != metadata2.mapping();
+    }
+
     /**
      * Returns <code>true</code> iff the cluster level blocks have changed between cluster states.
      * Note that this is an object reference equality test, not an equals test.

server/src/main/java/org/opensearch/indices/IndicesService.java

@@ -2129,6 +2129,19 @@ public void clearIndexShardCache(ShardId shardId, boolean queryCache, boolean fi
         }
     }
 
+    @Override
+    public void clearIndexShardCacheForAllShards(
+        Index index,
+        boolean queryCache,
+        boolean fieldDataCache,
+        boolean requestCache,
+        String... fields
+    ) {
+        for (IndexShard indexShard : indexService(index)) {
+            clearIndexShardCache(indexShard.shardId(), queryCache, fieldDataCache, requestCache, fields);
+        }
+    }
+
     /**
      * Returns a function which given an index name, returns a predicate which fields must match in order to be returned by get mappings,
      * get index, get field mappings and field capabilities API. Useful to filter the fields that such API return.

server/src/main/java/org/opensearch/indices/cluster/IndicesClusterStateService.java

@@ -645,6 +645,10 @@ private void updateIndices(ClusterChangedEvent event) {
                         }
                     }
                 }
+                if (ClusterChangedEvent.indexMappingMetadataChanged(newIndexMetadata, currentIndexMetadata)) {
+                    // Note this wipe is asynchronous and only marks keys for cleanup
+                    indicesService.clearIndexShardCacheForAllShards(index, false, false, true);
+                }
             }
         }
     }
@@ -1107,6 +1111,14 @@ default T getShardOrNull(ShardId shardId) {
         void processPendingDeletes(Index index, IndexSettings indexSettings, TimeValue timeValue) throws IOException, InterruptedException,
             ShardLockObtainFailedException;
 
+        void clearIndexShardCacheForAllShards(
+            Index index,
+            boolean queryCache,
+            boolean fieldDataCache,
+            boolean requestCache,
+            String... fields
+        );
+
         /**
          * Why the index was removed
          *

server/src/test/java/org/opensearch/indices/cluster/AbstractIndicesClusterStateServiceTestCase.java

@@ -296,6 +296,17 @@ private boolean hasIndex(Index index) {
         public Iterator<MockIndexService> iterator() {
             return indices.values().iterator();
         }
+
+        @Override
+        public void clearIndexShardCacheForAllShards(
+            Index index,
+            boolean queryCache,
+            boolean fieldDataCache,
+            boolean requestCache,
+            String... fields
+        ) {
+            return;
+        }
     }
 
     /**