Skip to content

Commit

Permalink
Core: Terms filter lookup caching should cache values, not filters.
Browse files Browse the repository at this point in the history 
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 elastic#9027
  • Loading branch information
@jpountz
jpountz committed Dec 24, 2014
1 parent 9712975 commit 66691a0
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 38 deletions.
14 changes: 2 additions & 12 deletions src/main/java/org/elasticsearch/index/query/TermsFilterParser.java
View file
Expand Up @@ -173,18 +173,8 @@ public Filter parse(QueryParseContext parseContext) throws IOException, QueryPar
}

// external lookup, use it
TermsLookup termsLookup = new TermsLookup(fieldMapper, lookupIndex, lookupType, lookupId, lookupRouting, lookupPath, parseContext);

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 || cache) {
filter = parseContext.cacheFilter(filter, cacheKey);
}
return filter;
TermsLookup termsLookup = new TermsLookup(lookupIndex, lookupType, lookupId, lookupRouting, lookupPath, parseContext);
terms.addAll(termsFilterCache.terms(termsLookup, lookupCache, cacheKey));
}

if (terms.isEmpty()) {
Expand Down
33 changes: 17 additions & 16 deletions src/main/java/org/elasticsearch/indices/cache/filter/terms/IndicesTermsFilterCache.java
View file
Expand Up @@ -22,16 +22,18 @@
import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
import com.google.common.cache.Weigher;
import org.apache.lucene.search.Filter;
import com.google.common.collect.ImmutableList;

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;
import org.elasticsearch.client.Client;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.component.AbstractComponent;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.lucene.search.Queries;
import org.elasticsearch.common.lucene.HashedBytesRef;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.ByteSizeUnit;
import org.elasticsearch.common.unit.ByteSizeValue;
Expand All @@ -48,7 +50,9 @@
*/
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;

Expand Down Expand Up @@ -77,10 +81,9 @@ public IndicesTermsFilterCache(Settings settings, Client client) {
this.cache = builder.build();
}

@Nullable
public Filter termsFilter(final TermsLookup lookup, boolean cacheLookup, @Nullable CacheKeyFilter.Key cacheKey) throws RuntimeException {
public List<Object> terms(final TermsLookup lookup, boolean cacheLookup, @Nullable CacheKeyFilter.Key cacheKey) throws RuntimeException {
if (!cacheLookup) {
return buildTermsFilterValue(lookup).filter;
return buildTermsFilterValue(lookup).values;
}

BytesRef key;
Expand All @@ -95,7 +98,7 @@ public Filter termsFilter(final TermsLookup lookup, boolean cacheLookup, @Nullab
public TermsFilterValue call() throws Exception {
return buildTermsFilterValue(lookup);
}
}).filter;
}).values;
} catch (ExecutionException e) {
if (e.getCause() instanceof RuntimeException) {
throw (RuntimeException) e.getCause();
Expand All @@ -113,17 +116,16 @@ TermsFilterValue buildTermsFilterValue(TermsLookup lookup) {
if (values.isEmpty()) {
return NO_TERMS;
}
Filter filter = lookup.getFieldMapper().termsFilter(values, lookup.getQueryParseContext());
return new TermsFilterValue(estimateSizeInBytes(values), filter);
return new TermsFilterValue(estimateSizeInBytes(values), ImmutableList.copyOf(values));
}

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;
}
Expand All @@ -149,14 +151,13 @@ public int weigh(BytesRef key, TermsFilterValue value) {
}
}

// 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;
}
}
}
12 changes: 2 additions & 10 deletions src/main/java/org/elasticsearch/indices/cache/filter/terms/TermsLookup.java
View file
Expand Up @@ -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;
Expand All @@ -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) {
this.fieldMapper = fieldMapper;
public TermsLookup(String index, String type, String id, String routing, String path, @Nullable QueryParseContext queryParseContext) {
this.index = index;
this.type = type;
this.id = id;
Expand All @@ -48,10 +44,6 @@ public TermsLookup(FieldMapper fieldMapper, String index, String type, String id
this.queryParseContext = queryParseContext;
}

public FieldMapper getFieldMapper() {
return fieldMapper;
}

public String getIndex() {
return index;
}
Expand All @@ -78,6 +70,6 @@ public QueryParseContext getQueryParseContext() {
}

public String toString() {
return fieldMapper.names().fullName() + ":" + index + "/" + type + "/" + id + "/" + path;
return index + "/" + type + "/" + id + "/" + path;
}
}

0 comments on commit 66691a0

Please sign in to comment.