Index: lucene/src/test/org/apache/lucene/search/TestCachingWrapperFilter.java =================================================================== --- lucene/src/test/org/apache/lucene/search/TestCachingWrapperFilter.java (revision 1213000) +++ lucene/src/test/org/apache/lucene/search/TestCachingWrapperFilter.java (working copy) @@ -30,8 +30,9 @@ import org.apache.lucene.index.Term; import org.apache.lucene.store.Directory; import org.apache.lucene.util.Bits; +import org.apache.lucene.util.FixedBitSet; import org.apache.lucene.util.LuceneTestCase; -import org.apache.lucene.util.FixedBitSet; +import org.apache.lucene.util._TestUtil; public class TestCachingWrapperFilter extends LuceneTestCase { @@ -164,6 +165,7 @@ // asserts below requires no unexpected merges: setMergePolicy(newLogMergePolicy(10)) ); + _TestUtil.keepFullyDeletedSegments(writer.w); // NOTE: cannot use writer.getReader because RIW (on // flipping a coin) may give us a newly opened reader, @@ -173,7 +175,7 @@ // same reason we don't wrap? IndexSearcher searcher = newSearcher(reader, false); - // add a doc, refresh the reader, and check that its there + // add a doc, refresh the reader, and check that it's there Document doc = new Document(); doc.add(newField("id", "1", StringField.TYPE_STORED)); writer.addDocument(doc); @@ -187,26 +189,81 @@ final Filter startFilter = new QueryWrapperFilter(new TermQuery(new Term("id", "1"))); - CachingWrapperFilter filter = new CachingWrapperFilter(startFilter); + // force cache to regenerate after deletions: + CachingWrapperFilter filter = new CachingWrapperFilter(startFilter, true); docs = searcher.search(new MatchAllDocsQuery(), filter, 1); + assertEquals("[query + filter] Should find a hit...", 1, docs.totalHits); - int missCount = filter.missCount; - assertTrue(missCount > 0); + Query constantScore = new ConstantScoreQuery(filter); docs = searcher.search(constantScore, 1); assertEquals("[just filter] Should find a hit...", 1, docs.totalHits); + + // make sure we get a cache hit when we reopen reader + // that had no change to deletions + + // fake delete (deletes nothing): + writer.deleteDocuments(new Term("foo", "bar")); + + IndexReader oldReader = reader; + reader = refreshReader(reader); + assertTrue(reader == oldReader); + int missCount = filter.missCount; + docs = searcher.search(constantScore, 1); + assertEquals("[just filter] Should find a hit...", 1, docs.totalHits); + + // cache hit: assertEquals(missCount, filter.missCount); + // now delete the doc, refresh the reader, and see that it's not there + writer.deleteDocuments(new Term("id", "1")); + // NOTE: important to hold ref here so GC doesn't clear // the cache entry! Else the assert below may sometimes // fail: - IndexReader oldReader = reader; + oldReader = reader; + reader = refreshReader(reader); + searcher.close(); + searcher = newSearcher(reader, false); + + missCount = filter.missCount; + docs = searcher.search(new MatchAllDocsQuery(), filter, 1); + assertEquals("[query + filter] Should *not* find a hit...", 0, docs.totalHits); + + // cache miss, because we asked CWF to recache when + // deletes changed: + assertEquals(missCount+1, filter.missCount); + docs = searcher.search(constantScore, 1); + assertEquals("[just filter] Should *not* find a hit...", 0, docs.totalHits); + + // apply deletes dynamically: + filter = new CachingWrapperFilter(startFilter); writer.addDocument(doc); reader = refreshReader(reader); searcher.close(); searcher = newSearcher(reader, false); + + docs = searcher.search(new MatchAllDocsQuery(), filter, 1); + assertEquals("[query + filter] Should find a hit...", 1, docs.totalHits); + missCount = filter.missCount; + assertTrue(missCount > 0); + constantScore = new ConstantScoreQuery(filter); + docs = searcher.search(constantScore, 1); + assertEquals("[just filter] Should find a hit...", 1, docs.totalHits); + assertEquals(missCount, filter.missCount); + + writer.addDocument(doc); + + // NOTE: important to hold ref here so GC doesn't clear + // the cache entry! Else the assert below may sometimes + // fail: + oldReader = reader; + + reader = refreshReader(reader); + searcher.close(); + searcher = newSearcher(reader, false); docs = searcher.search(new MatchAllDocsQuery(), filter, 1); assertEquals("[query + filter] Should find 2 hits...", 2, docs.totalHits); @@ -218,11 +275,6 @@ assertEquals("[just filter] Should find a hit...", 2, docs.totalHits); assertEquals(missCount, filter.missCount); - // NOTE: important to hold ref here so GC doesn't clear - // the cache entry! Else the assert below may sometimes - // fail: - IndexReader oldReader2 = reader; - // now delete the doc, refresh the reader, and see that it's not there writer.deleteDocuments(new Term("id", "1")); @@ -232,10 +284,12 @@ docs = searcher.search(new MatchAllDocsQuery(), filter, 1); assertEquals("[query + filter] Should *not* find a hit...", 0, docs.totalHits); + // CWF reused the same entry (it dynamically applied the deletes): assertEquals(missCount, filter.missCount); docs = searcher.search(constantScore, 1); assertEquals("[just filter] Should *not* find a hit...", 0, docs.totalHits); + // CWF reused the same entry (it dynamically applied the deletes): assertEquals(missCount, filter.missCount); // NOTE: silliness to make sure JRE does not eliminate @@ -243,7 +297,6 @@ // CachingWrapperFilter's WeakHashMap from dropping the // entry: assertTrue(oldReader != null); - assertTrue(oldReader2 != null); searcher.close(); reader.close(); Index: lucene/src/java/org/apache/lucene/search/CachingWrapperFilter.java =================================================================== --- lucene/src/java/org/apache/lucene/search/CachingWrapperFilter.java (revision 1213000) +++ lucene/src/java/org/apache/lucene/search/CachingWrapperFilter.java (working copy) @@ -38,28 +38,27 @@ Filter filter; protected final FilterCache cache; + private final boolean recacheDeletes; - static class FilterCache { + private static class FilterCache { /** * A transient Filter cache (package private because of test) */ - // NOTE: not final so that we can dynamically re-init - // after de-serialize - transient Map cache; + private final Map> cache = new WeakHashMap>(); - public synchronized T get(IndexReader reader, Object coreKey) throws IOException { - T value; - - if (cache == null) { - cache = new WeakHashMap(); + public synchronized T get(IndexReader reader, Object coreKey, Object coreSubKey) throws IOException { + Map innerCache = cache.get(coreKey); + if (innerCache == null) { + innerCache = new WeakHashMap(); + cache.put(coreKey, innerCache); } - return cache.get(coreKey); + return innerCache.get(coreSubKey); } - public synchronized void put(Object coreKey, T value) { - cache.put(coreKey, value); + public synchronized void put(Object coreKey, Object coreSubKey, T value) { + cache.get(coreKey).put(coreSubKey, value); } } @@ -67,7 +66,19 @@ * @param filter Filter to cache results of */ public CachingWrapperFilter(Filter filter) { + this(filter, false); + } + + /** Wraps another filter's result and caches it. If + * recacheDeletes is true, then new deletes (for example + * after {@link IndexReader#openIfChanged}) will be AND'd + * and cached again. + * + * @param filter Filter to cache results of + */ + public CachingWrapperFilter(Filter filter, boolean recacheDeletes) { this.filter = filter; + this.recacheDeletes = recacheDeletes; cache = new FilterCache(); } @@ -106,33 +117,48 @@ final IndexReader reader = context.reader; final Object coreKey = reader.getCoreCacheKey(); - DocIdSet docIdSet = cache.get(reader, coreKey); + // Only cache if incoming acceptDocs is == live docs; + // if Lucene passes in more interesting acceptDocs in + // the future we don't want to over-cache: + final boolean doCacheSubAcceptDocs = recacheDeletes && acceptDocs == reader.getLiveDocs(); + + final Bits subAcceptDocs; + if (doCacheSubAcceptDocs) { + subAcceptDocs = acceptDocs; + } else { + subAcceptDocs = null; + } + + DocIdSet docIdSet = cache.get(reader, coreKey, subAcceptDocs); if (docIdSet != null) { hitCount++; } else { missCount++; - // cache miss: we use no acceptDocs here - // (this saves time on building DocIdSet, the acceptDocs will be applied on the cached set) - docIdSet = docIdSetToCache(filter.getDocIdSet(context, null/**!!!*/), reader); - cache.put(coreKey, docIdSet); + docIdSet = docIdSetToCache(filter.getDocIdSet(context, subAcceptDocs), reader); + cache.put(coreKey, subAcceptDocs, docIdSet); } - - return BitsFilteredDocIdSet.wrap(docIdSet, acceptDocs); + + if (doCacheSubAcceptDocs) { + return docIdSet; + } else { + return BitsFilteredDocIdSet.wrap(docIdSet, acceptDocs); + } } @Override public String toString() { - return "CachingWrapperFilter("+filter+")"; + return "CachingWrapperFilter("+filter+",recacheDeletes=" + recacheDeletes + ")"; } @Override public boolean equals(Object o) { if (!(o instanceof CachingWrapperFilter)) return false; - return this.filter.equals(((CachingWrapperFilter)o).filter); + final CachingWrapperFilter other = (CachingWrapperFilter) o; + return this.filter.equals(other.filter) && this.recacheDeletes == other.recacheDeletes; } @Override public int hashCode() { - return filter.hashCode() ^ 0x1117BF25; + return (filter.hashCode() ^ 0x1117BF25) + (recacheDeletes ? 0 : 1); } } Index: lucene/src/test-framework/java/org/apache/lucene/search/CachingWrapperFilterHelper.java =================================================================== --- lucene/src/test-framework/java/org/apache/lucene/search/CachingWrapperFilterHelper.java (revision 1213000) +++ lucene/src/test-framework/java/org/apache/lucene/search/CachingWrapperFilterHelper.java (working copy) @@ -1,75 +0,0 @@ -package org.apache.lucene.search; - -/** - * 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. - */ - -import java.io.IOException; - -import junit.framework.Assert; - -import org.apache.lucene.index.IndexReader.AtomicReaderContext; -import org.apache.lucene.util.Bits; - -/** - * A unit test helper class to test when the filter is getting cached and when it is not. - */ -public class CachingWrapperFilterHelper extends CachingWrapperFilter { - - private boolean shouldHaveCache = false; - - /** - * @param filter Filter to cache results of - */ - public CachingWrapperFilterHelper(Filter filter) { - super(filter); - } - - public void setShouldHaveCache(boolean shouldHaveCache) { - this.shouldHaveCache = shouldHaveCache; - } - - @Override - public synchronized DocIdSet getDocIdSet(AtomicReaderContext context, Bits acceptDocs) throws IOException { - - final int saveMissCount = missCount; - DocIdSet docIdSet = super.getDocIdSet(context, acceptDocs); - - if (shouldHaveCache) { - Assert.assertEquals("Cache should have data ", saveMissCount, missCount); - } else { - Assert.assertTrue("Cache should be null " + docIdSet, missCount > saveMissCount); - } - - return docIdSet; - } - - @Override - public String toString() { - return "CachingWrapperFilterHelper("+filter+")"; - } - - @Override - public boolean equals(Object o) { - if (!(o instanceof CachingWrapperFilterHelper)) return false; - return this.filter.equals(o); - } - - @Override - public int hashCode() { - return this.filter.hashCode() ^ 0x5525aacb; - } -}