From 0f1738e26571f9f61d12ab2b7fd2da8e46c0bef8 Mon Sep 17 00:00:00 2001 From: Shai Erera Date: Mon, 1 Apr 2013 04:49:36 +0000 Subject: [PATCH] LUCENE-4893: facet counts were multiplied as many times as FacetsCollector.getFacetResults was called git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1463085 13f79535-47bb-0310-9956-ffa450edef68 --- lucene/CHANGES.txt | 3 + .../lucene/facet/search/FacetResult.java | 1 - .../facet/search/FacetsAccumulator.java | 16 ++-- .../lucene/facet/search/FacetsCollector.java | 38 +++++++--- .../facet/search/TestFacetsCollector.java | 74 +++++++++++++++++++ 5 files changed, 116 insertions(+), 16 deletions(-) diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index 1258f0efa3c..0c2bfdac1b2 100644 --- a/lucene/CHANGES.txt +++ b/lucene/CHANGES.txt @@ -189,6 +189,9 @@ Bug Fixes IndexDeletionPolicy and InfoStream in order to make an IndexWriterConfig and its clone fully independent. (Adrien Grand) +* LUCENE-4893: Facet counts were multiplied as many times as + FacetsCollector.getFacetResults() is called. (Shai Erera) + Documentation * LUCENE-4841: Added example SimpleSortedSetFacetsExample to show how diff --git a/lucene/facet/src/java/org/apache/lucene/facet/search/FacetResult.java b/lucene/facet/src/java/org/apache/lucene/facet/search/FacetResult.java index 3d9732d8d51..0d0522f1b8f 100644 --- a/lucene/facet/src/java/org/apache/lucene/facet/search/FacetResult.java +++ b/lucene/facet/src/java/org/apache/lucene/facet/search/FacetResult.java @@ -1,6 +1,5 @@ package org.apache.lucene.facet.search; - /* * Licensed to the Apache Software Foundation (ASF) under one or more * contributor license agreements. See the NOTICE file distributed with diff --git a/lucene/facet/src/java/org/apache/lucene/facet/search/FacetsAccumulator.java b/lucene/facet/src/java/org/apache/lucene/facet/search/FacetsAccumulator.java index 0bc39a3d009..5aff919dfc1 100644 --- a/lucene/facet/src/java/org/apache/lucene/facet/search/FacetsAccumulator.java +++ b/lucene/facet/src/java/org/apache/lucene/facet/search/FacetsAccumulator.java @@ -81,6 +81,14 @@ public class FacetsAccumulator { return new FacetsAccumulator(fsp, indexReader, taxoReader); } + private static FacetResult emptyResult(int ordinal, FacetRequest fr) { + FacetResultNode root = new FacetResultNode(); + root.ordinal = ordinal; + root.label = fr.categoryPath; + root.value = 0; + return new FacetResult(fr, root, 0); + } + /** * Initializes the accumulator with the given parameters as well as * {@link FacetArrays}. Note that the accumulator doesn't call @@ -173,12 +181,8 @@ public class FacetsAccumulator { for (FacetRequest fr : searchParams.facetRequests) { int rootOrd = taxonomyReader.getOrdinal(fr.categoryPath); if (rootOrd == TaxonomyReader.INVALID_ORDINAL) { // category does not exist - // Add empty FacetResult: - FacetResultNode root = new FacetResultNode(); - root.ordinal = TaxonomyReader.INVALID_ORDINAL; - root.label = fr.categoryPath; - root.value = 0; - res.add(new FacetResult(fr, root, 0)); + // Add empty FacetResult + res.add(emptyResult(rootOrd, fr)); continue; } CategoryListParams clp = searchParams.indexingParams.getCategoryListParams(fr.categoryPath); diff --git a/lucene/facet/src/java/org/apache/lucene/facet/search/FacetsCollector.java b/lucene/facet/src/java/org/apache/lucene/facet/search/FacetsCollector.java index 500ad5dfc4c..7d04fc4d81b 100644 --- a/lucene/facet/src/java/org/apache/lucene/facet/search/FacetsCollector.java +++ b/lucene/facet/src/java/org/apache/lucene/facet/search/FacetsCollector.java @@ -87,7 +87,7 @@ public abstract class FacetsCollector extends Collector { } @Override - public final void setNextReader(AtomicReaderContext context) throws IOException { + protected final void doSetNextReader(AtomicReaderContext context) throws IOException { if (bits != null) { matchingDocs.add(new MatchingDocs(this.context, bits, totalHits, scores)); } @@ -133,7 +133,7 @@ public abstract class FacetsCollector extends Collector { public final void setScorer(Scorer scorer) throws IOException {} @Override - public final void setNextReader(AtomicReaderContext context) throws IOException { + protected final void doSetNextReader(AtomicReaderContext context) throws IOException { if (bits != null) { matchingDocs.add(new MatchingDocs(this.context, bits, totalHits, null)); } @@ -183,6 +183,7 @@ public abstract class FacetsCollector extends Collector { } private final FacetsAccumulator accumulator; + private List cachedResults; protected final List matchingDocs = new ArrayList(); @@ -196,15 +197,24 @@ public abstract class FacetsCollector extends Collector { */ protected abstract void finish(); + /** Performs the actual work of {@link #setNextReader(AtomicReaderContext)}. */ + protected abstract void doSetNextReader(AtomicReaderContext context) throws IOException; + /** * Returns a {@link FacetResult} per {@link FacetRequest} set in - * {@link FacetSearchParams}. Note that if one of the {@link FacetRequest - * requests} is for a {@link CategoryPath} that does not exist in the taxonomy, - * no matching {@link FacetResult} will be returned. + * {@link FacetSearchParams}. Note that if a {@link FacetRequest} defines a + * {@link CategoryPath} which does not exist in the taxonomy, an empty + * {@link FacetResult} will be returned for it. */ public final List getFacetResults() throws IOException { - finish(); - return accumulator.accumulate(matchingDocs); + // LUCENE-4893: if results are not cached, counts are multiplied as many + // times as this method is called. + if (cachedResults == null) { + finish(); + cachedResults = accumulator.accumulate(matchingDocs); + } + + return cachedResults; } /** @@ -218,12 +228,22 @@ public abstract class FacetsCollector extends Collector { /** * Allows to reuse the collector between search requests. This method simply - * clears all collected documents (and scores) information, and does not - * attempt to reuse allocated memory spaces. + * clears all collected documents (and scores) information (as well as cached + * results), and does not attempt to reuse allocated memory spaces. */ public final void reset() { finish(); matchingDocs.clear(); + cachedResults = null; } + @Override + public final void setNextReader(AtomicReaderContext context) throws IOException { + // clear cachedResults - needed in case someone called getFacetResults() + // before doing a search and didn't call reset(). Defensive code to prevent + // traps. + cachedResults = null; + doSetNextReader(context); + } + } diff --git a/lucene/facet/src/test/org/apache/lucene/facet/search/TestFacetsCollector.java b/lucene/facet/src/test/org/apache/lucene/facet/search/TestFacetsCollector.java index b9ae776d73e..8e4ff533bd4 100644 --- a/lucene/facet/src/test/org/apache/lucene/facet/search/TestFacetsCollector.java +++ b/lucene/facet/src/test/org/apache/lucene/facet/search/TestFacetsCollector.java @@ -230,4 +230,78 @@ public class TestFacetsCollector extends FacetTestCase { IOUtils.close(taxo, taxoDir, r, indexDir); } + @Test + public void testGetFacetResultsTwice() throws Exception { + // LUCENE-4893: counts were multiplied as many times as getFacetResults was called. + Directory indexDir = newDirectory(); + Directory taxoDir = newDirectory(); + + TaxonomyWriter taxonomyWriter = new DirectoryTaxonomyWriter(taxoDir); + IndexWriter iw = new IndexWriter(indexDir, newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random()))); + + FacetFields facetFields = new FacetFields(taxonomyWriter); + Document doc = new Document(); + facetFields.addFields(doc, Arrays.asList(new CategoryPath("a/1", '/'), new CategoryPath("b/1", '/'))); + iw.addDocument(doc); + taxonomyWriter.close(); + iw.close(); + + DirectoryReader r = DirectoryReader.open(indexDir); + DirectoryTaxonomyReader taxo = new DirectoryTaxonomyReader(taxoDir); + + FacetSearchParams fsp = new FacetSearchParams( + new CountFacetRequest(new CategoryPath("a"), 10), + new CountFacetRequest(new CategoryPath("b"), 10)); + final FacetsAccumulator fa = random().nextBoolean() ? new FacetsAccumulator(fsp, r, taxo) : new StandardFacetsAccumulator(fsp, r, taxo); + final FacetsCollector fc = FacetsCollector.create(fa); + new IndexSearcher(r).search(new MatchAllDocsQuery(), fc); + + List res1 = fc.getFacetResults(); + List res2 = fc.getFacetResults(); + assertSame("calling getFacetResults twice should return the exact same result", res1, res2); + + IOUtils.close(taxo, taxoDir, r, indexDir); + } + + @Test + public void testReset() throws Exception { + Directory indexDir = newDirectory(); + Directory taxoDir = newDirectory(); + + TaxonomyWriter taxonomyWriter = new DirectoryTaxonomyWriter(taxoDir); + IndexWriter iw = new IndexWriter(indexDir, newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random()))); + + FacetFields facetFields = new FacetFields(taxonomyWriter); + Document doc = new Document(); + facetFields.addFields(doc, Arrays.asList(new CategoryPath("a/1", '/'), new CategoryPath("b/1", '/'))); + iw.addDocument(doc); + taxonomyWriter.close(); + iw.close(); + + DirectoryReader r = DirectoryReader.open(indexDir); + DirectoryTaxonomyReader taxo = new DirectoryTaxonomyReader(taxoDir); + + FacetSearchParams fsp = new FacetSearchParams( + new CountFacetRequest(new CategoryPath("a"), 10), + new CountFacetRequest(new CategoryPath("b"), 10)); + final FacetsAccumulator fa = random().nextBoolean() ? new FacetsAccumulator(fsp, r, taxo) : new StandardFacetsAccumulator(fsp, r, taxo); + final FacetsCollector fc = FacetsCollector.create(fa); + // this should populate the cached results, but doing search should clear the cache + fc.getFacetResults(); + new IndexSearcher(r).search(new MatchAllDocsQuery(), fc); + + List res1 = fc.getFacetResults(); + // verify that we didn't get the cached result + assertEquals(2, res1.size()); + for (FacetResult res : res1) { + assertEquals(1, res.getFacetResultNode().subResults.size()); + assertEquals(1, (int) res.getFacetResultNode().subResults.get(0).value); + } + fc.reset(); + List res2 = fc.getFacetResults(); + assertNotSame("reset() should clear the cached results", res1, res2); + + IOUtils.close(taxo, taxoDir, r, indexDir); + } + }