Core: Terms filter lookup caching should cache values, not filters.

The terms filter lookup mechanism today caches filters. Because of this, the
cache values depend on two things: the values that can be found in the lookup
index AND the mapping of the local index, since changing the mapping can change
the way that the filter is parsed. We should make the cache depend solely on
the content of the lookup index.

For instance the issue I was seeing was due to the following scenario:
 - create index1 with _id indexed
 - run terms filter with lookup, the parsed filter looks like `_id: 1 OR _id: 2`
 - remove index1
 - create index1 with _id not indexed
 - run terms filter without lookup, the parsed filter is `_uid: type#1 OR _uid: type#2` (the _id field mapper knows how to use the _uid field when _id is not indexed)
 - run terms filter with lookup, the filter is fetched from the cache: `_id: 1 OR _id: 2` but does not match anything since `_id` is not indexed.

Close #9027

src/main/java/org/elasticsearch/index/query/TermsFilterParser.java

@@ -188,18 +188,8 @@ public class TermsFilterParser implements FilterParser {
             }
 
             // external lookup, use it
-            TermsLookup termsLookup = new TermsLookup(fieldMapper, lookupIndex, lookupType, lookupId, lookupRouting, lookupPath, parseContext);
+            TermsLookup termsLookup = new TermsLookup(lookupIndex, lookupType, lookupId, lookupRouting, lookupPath, parseContext);
-
+            terms.addAll(termsFilterCache.terms(termsLookup, lookupCache, cacheKey));
-            Filter filter = termsFilterCache.termsFilter(termsLookup, lookupCache, cacheKey);
-            if (filter == null) {
-                return null;
-            }
-
-            // cache the whole filter by default, or if explicitly told to
-            if (cache != null) {
-                filter = parseContext.cacheFilter(filter, cacheKey, cache);
-            }
-            return filter;
         }
 
         if (terms.isEmpty()) {

src/main/java/org/elasticsearch/indices/cache/filter/terms/IndicesTermsFilterCache.java

@@ -22,9 +22,10 @@ package org.elasticsearch.indices.cache.filter.terms;
 import com.google.common.cache.Cache;
 import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.Weigher;
+import com.google.common.collect.ImmutableList;
 
-import org.apache.lucene.search.Filter;
 import org.apache.lucene.util.BytesRef;
+import org.apache.lucene.util.RamUsageEstimator;
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.action.get.GetRequest;
 import org.elasticsearch.action.get.GetResponse;
@@ -33,7 +34,6 @@ import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.component.AbstractComponent;
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.lucene.HashedBytesRef;
-import org.elasticsearch.common.lucene.search.Queries;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.ByteSizeUnit;
 import org.elasticsearch.common.unit.ByteSizeValue;
@@ -49,7 +49,9 @@ import java.util.concurrent.TimeUnit;
  */
 public class IndicesTermsFilterCache extends AbstractComponent {
 
-    private static TermsFilterValue NO_TERMS = new TermsFilterValue(0, Queries.MATCH_NO_FILTER);
+    private static final long BASE_RAM_BYTES_STRING = RamUsageEstimator.shallowSizeOfInstance(String.class) + RamUsageEstimator.NUM_BYTES_OBJECT_HEADER;
+    private static final long BASE_RAM_BYTES_BYTES_REF = RamUsageEstimator.shallowSizeOfInstance(BytesRef.class) + RamUsageEstimator.NUM_BYTES_OBJECT_HEADER;
+    private static final TermsFilterValue NO_TERMS = new TermsFilterValue(0, ImmutableList.of());
 
     private final Client client;
 
@@ -78,10 +80,9 @@ public class IndicesTermsFilterCache extends AbstractComponent {
         this.cache = builder.build();
     }
 
-    @Nullable
+    public List<Object> terms(final TermsLookup lookup, boolean cacheLookup, @Nullable HashedBytesRef cacheKey) throws RuntimeException {
-    public Filter termsFilter(final TermsLookup lookup, boolean cacheLookup, @Nullable HashedBytesRef cacheKey) throws RuntimeException {
         if (!cacheLookup) {
-            return buildTermsFilterValue(lookup).filter;
+            return buildTermsFilterValue(lookup).values;
         }
 
         HashedBytesRef key;
@@ -96,7 +97,7 @@ public class IndicesTermsFilterCache extends AbstractComponent {
                 public TermsFilterValue call() throws Exception {
                     return buildTermsFilterValue(lookup);
                 }
-            }).filter;
+            }).values;
         } catch (ExecutionException e) {
             if (e.getCause() instanceof RuntimeException) {
                 throw (RuntimeException) e.getCause();
@@ -114,17 +115,16 @@ public class IndicesTermsFilterCache extends AbstractComponent {
         if (values.isEmpty()) {
             return NO_TERMS;
         }
-        Filter filter = lookup.getFieldMapper().termsFilter(values, lookup.getQueryParseContext());
+        return new TermsFilterValue(estimateSizeInBytes(values), ImmutableList.copyOf(values));
-        return new TermsFilterValue(estimateSizeInBytes(values), filter);
     }
 
     long estimateSizeInBytes(List<Object> terms) {
-        long size = 8;
+        long size = 8 + terms.size() * RamUsageEstimator.NUM_BYTES_OBJECT_REF;
         for (Object term : terms) {
             if (term instanceof BytesRef) {
-                size += ((BytesRef) term).length;
+                size += BASE_RAM_BYTES_BYTES_REF + ((BytesRef) term).length;
             } else if (term instanceof String) {
-                size += ((String) term).length() / 2;
+                size += BASE_RAM_BYTES_STRING + ((String) term).length() * RamUsageEstimator.NUM_BYTES_CHAR;
             } else {
                 size += 4;
             }
@@ -150,14 +150,13 @@ public class IndicesTermsFilterCache extends AbstractComponent {
         }
     }
 
-    // TODO: if TermsFilter exposed sizeInBytes, we won't need this wrapper
     static class TermsFilterValue {
         public final long sizeInBytes;
-        public final Filter filter;
+        public final ImmutableList<Object> values;
 
-        TermsFilterValue(long sizeInBytes, Filter filter) {
+        TermsFilterValue(long sizeInBytes, ImmutableList<Object> values) {
             this.sizeInBytes = sizeInBytes;
-            this.filter = filter;
+            this.values = values;
         }
     }
 }

src/main/java/org/elasticsearch/indices/cache/filter/terms/TermsLookup.java

@@ -20,15 +20,12 @@
 package org.elasticsearch.indices.cache.filter.terms;
 
 import org.elasticsearch.common.Nullable;
-import org.elasticsearch.index.mapper.FieldMapper;
 import org.elasticsearch.index.query.QueryParseContext;
 
 /**
  */
 public class TermsLookup {
 
-    private final FieldMapper fieldMapper;
-
     private final String index;
     private final String type;
     private final String id;
@@ -38,8 +35,7 @@ public class TermsLookup {
     @Nullable
     private final QueryParseContext queryParseContext;
 
-    public TermsLookup(FieldMapper fieldMapper, String index, String type, String id, String routing, String path, @Nullable QueryParseContext queryParseContext) {
+    public TermsLookup(String index, String type, String id, String routing, String path, @Nullable QueryParseContext queryParseContext) {
-        this.fieldMapper = fieldMapper;
         this.index = index;
         this.type = type;
         this.id = id;
@@ -48,10 +44,6 @@ public class TermsLookup {
         this.queryParseContext = queryParseContext;
     }
 
-    public FieldMapper getFieldMapper() {
-        return fieldMapper;
-    }
-
     public String getIndex() {
         return index;
     }
@@ -78,6 +70,6 @@ public class TermsLookup {
     }
 
     public String toString() {
-        return fieldMapper.names().fullName() + ":" + index + "/" + type + "/" + id + "/" + path;
+        return index + "/" + type + "/" + id + "/" + path;
     }
 }