From a89dde8bac344f676f5deadd4a49e12ff614d3ca Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Fri, 29 Mar 2013 16:48:36 +0100 Subject: [PATCH] Fixed `bool` filter bugs: * In the case only should clauses were specified with specific type of filters, the first clause determined which documents matched. * In some cases the minimum at least 1 should clause should match behaviour was broken. --- .../common/lucene/search/XBooleanFilter.java | 75 ++-- .../search/query/SimpleQueryTests.java | 57 +++ .../search/XBooleanFilterLuceneTests.java | 370 ++++++++++++++++++ .../lucene/search/XBooleanFilterTests.java | 132 ++++++- 4 files changed, 609 insertions(+), 25 deletions(-) create mode 100644 src/test/java/org/elasticsearch/test/unit/common/lucene/search/XBooleanFilterLuceneTests.java diff --git a/src/main/java/org/elasticsearch/common/lucene/search/XBooleanFilter.java b/src/main/java/org/elasticsearch/common/lucene/search/XBooleanFilter.java index 77b5556ec21..709b86100d7 100644 --- a/src/main/java/org/elasticsearch/common/lucene/search/XBooleanFilter.java +++ b/src/main/java/org/elasticsearch/common/lucene/search/XBooleanFilter.java @@ -78,10 +78,13 @@ public class XBooleanFilter extends Filter implements Iterable { List results = new ArrayList(clauses.size()); boolean hasShouldClauses = false; boolean hasNonEmptyShouldClause = false; + boolean hasMustClauses = false; + boolean hasMustNotClauses = false; for (int i = 0; i < clauses.size(); i++) { FilterClause clause = clauses.get(i); DocIdSet set = clause.getFilter().getDocIdSet(context, acceptDocs); if (clause.getOccur() == Occur.MUST) { + hasMustClauses = true; if (DocIdSets.isEmpty(set)) { return null; } @@ -92,6 +95,7 @@ public class XBooleanFilter extends Filter implements Iterable { } hasNonEmptyShouldClause = true; } else if (clause.getOccur() == Occur.MUST_NOT) { + hasMustNotClauses = true; if (DocIdSets.isEmpty(set)) { // we mark empty ones as null for must_not, handle it in the next run... results.add(new ResultClause(null, null, clause)); @@ -110,6 +114,7 @@ public class XBooleanFilter extends Filter implements Iterable { } // now, go over the clauses and apply the "fast" ones... + hasNonEmptyShouldClause = false; boolean hasBits = false; for (int i = 0; i < results.size(); i++) { ResultClause clause = results.get(i); @@ -123,6 +128,7 @@ public class XBooleanFilter extends Filter implements Iterable { if (it == null) { continue; } + hasNonEmptyShouldClause = true; if (res == null) { res = new FixedBitSet(reader.maxDoc()); } @@ -153,40 +159,40 @@ public class XBooleanFilter extends Filter implements Iterable { } if (!hasBits) { - return res; + if (hasShouldClauses && !hasNonEmptyShouldClause) { + return null; + } else { + return res; + } } // we have some clauses with bits, apply them... // we let the "res" drive the computation, and check Bits for that + List orClauses = new ArrayList(); for (int i = 0; i < results.size(); i++) { ResultClause clause = results.get(i); - // we apply bits in based ones (slow) in the second run if (clause.bits == null) { continue; } if (clause.clause.getOccur() == Occur.SHOULD) { - if (res == null) { - DocIdSetIterator it = clause.docIdSet.iterator(); - if (it == null) { - continue; - } - res = new FixedBitSet(reader.maxDoc()); - res.or(it); + if (hasMustClauses || hasMustNotClauses) { + orClauses.add(clause); } else { - Bits bits = clause.bits; - // use the "res" to drive the iteration - int lastSetDoc = res.nextSetBit(0); - DocIdSetIterator it = res.iterator(); - for (int setDoc = it.nextDoc(); setDoc != DocIdSetIterator.NO_MORE_DOCS; setDoc = it.nextDoc()) { - int diff = setDoc - lastSetDoc; - if (diff > 1) { - for (int unsetDoc = lastSetDoc + 1; unsetDoc < setDoc; unsetDoc++) { - if (bits.get(unsetDoc)) { - res.set(unsetDoc); - } + if (res == null) { + DocIdSetIterator it = clause.docIdSet.iterator(); + if (it == null) { + continue; + } + hasNonEmptyShouldClause = true; + res = new FixedBitSet(reader.maxDoc()); + res.or(it); + } else if (!hasMustNotClauses && !hasMustClauses) { + for (int doc = 0; doc < reader.maxDoc(); doc++) { + if (!res.get(doc) && clause.bits.get(doc)) { + hasNonEmptyShouldClause = true; + res.set(doc); } } - lastSetDoc = setDoc; } } } else if (clause.clause.getOccur() == Occur.MUST) { @@ -229,7 +235,32 @@ public class XBooleanFilter extends Filter implements Iterable { } } - return res; + // From a boolean_logic behavior point of view a should clause doesn't have impact on a bool filter if there + // is already a must or must_not clause. However in the current ES bool filter behaviour all should clauses + // must match in order for a doc to be a match. What we do here is checking if matched docs match with any + // should filter. TODO: Add an option to have disable minimum_should_match=1 behaviour + if (!orClauses.isEmpty()) { + DocIdSetIterator it = res.iterator(); + for (int setDoc = it.nextDoc(); setDoc != DocIdSetIterator.NO_MORE_DOCS; setDoc = it.nextDoc()) { + boolean match = false; + for (ResultClause orClause : orClauses) { + if (orClause.bits.get(setDoc)) { + match = true; + hasNonEmptyShouldClause = true; + } + } + if (!match) { + res.clear(setDoc); + } + } + } + + if (hasShouldClauses && !hasNonEmptyShouldClause) { + return null; + } else { + return res; + } + } /** diff --git a/src/test/java/org/elasticsearch/test/integration/search/query/SimpleQueryTests.java b/src/test/java/org/elasticsearch/test/integration/search/query/SimpleQueryTests.java index 987e430d3e0..807dd5ca0ab 100644 --- a/src/test/java/org/elasticsearch/test/integration/search/query/SimpleQueryTests.java +++ b/src/test/java/org/elasticsearch/test/integration/search/query/SimpleQueryTests.java @@ -1087,4 +1087,61 @@ public class SimpleQueryTests extends AbstractNodesTests { assertThat(searchResponse.getHits().getTotalHits(), equalTo(1l)); assertThat(searchResponse.getHits().getAt(0).getId(), equalTo("1")); } + + @Test + public void testNumericRangeFilter_2826() throws Exception { + client.admin().indices().prepareDelete().execute().actionGet(); + client.admin().indices().prepareCreate("test").setSettings( + ImmutableSettings.settingsBuilder() + .put("index.number_of_shards", 1) + .put("index.number_of_replicas", 0) + ) + .addMapping("type1", jsonBuilder().startObject().startObject("type1").startObject("properties") + .startObject("num_byte").field("type", "byte").endObject() + .startObject("num_short").field("type", "short").endObject() + .startObject("num_integer").field("type", "integer").endObject() + .startObject("num_long").field("type", "long").endObject() + .startObject("num_float").field("type", "float").endObject() + .startObject("num_double").field("type", "double").endObject() + .endObject().endObject().endObject()) + .execute().actionGet(); + + client.prepareIndex("test", "type1", "1").setSource(jsonBuilder().startObject() + .field("num_long", 1) + .endObject()) + .execute().actionGet(); + + client.prepareIndex("test", "type1", "2").setSource(jsonBuilder().startObject() + .field("num_long", 2) + .endObject()) + .execute().actionGet(); + + client.prepareIndex("test", "type1", "3").setSource(jsonBuilder().startObject() + .field("num_long", 3) + .endObject()) + .execute().actionGet(); + + client.prepareIndex("test", "type1", "4").setSource(jsonBuilder().startObject() + .field("num_long", 4) + .endObject()) + .execute().actionGet(); + + client.admin().indices().prepareRefresh().execute().actionGet(); + SearchResponse response = client.prepareSearch("test").setFilter( + FilterBuilders.boolFilter() + .should(FilterBuilders.rangeFilter("num_long").from(1).to(2)) + .should(FilterBuilders.rangeFilter("num_long").from(3).to(4)) + ).execute().actionGet(); + assertThat(response.getHits().totalHits(), equalTo(4l)); + + // This made 2826 fail! (only with bit based filters) + response = client.prepareSearch("test").setFilter( + FilterBuilders.boolFilter() + .should(FilterBuilders.numericRangeFilter("num_long").from(1).to(2)) + .should(FilterBuilders.numericRangeFilter("num_long").from(3).to(4)) + ).execute().actionGet(); + + assertThat(response.getHits().totalHits(), equalTo(4l)); + } + } diff --git a/src/test/java/org/elasticsearch/test/unit/common/lucene/search/XBooleanFilterLuceneTests.java b/src/test/java/org/elasticsearch/test/unit/common/lucene/search/XBooleanFilterLuceneTests.java new file mode 100644 index 00000000000..2ab6f666208 --- /dev/null +++ b/src/test/java/org/elasticsearch/test/unit/common/lucene/search/XBooleanFilterLuceneTests.java @@ -0,0 +1,370 @@ +package org.elasticsearch.test.unit.common.lucene.search; + +import org.apache.lucene.analysis.core.WhitespaceAnalyzer; +import org.apache.lucene.document.Document; +import org.apache.lucene.document.Field; +import org.apache.lucene.document.TextField; +import org.apache.lucene.index.*; +import org.apache.lucene.queries.FilterClause; +import org.apache.lucene.queries.TermsFilter; +import org.apache.lucene.search.*; +import org.apache.lucene.store.Directory; +import org.apache.lucene.store.RAMDirectory; +import org.apache.lucene.util.Bits; +import org.apache.lucene.util.FixedBitSet; +import org.elasticsearch.common.lucene.Lucene; +import org.elasticsearch.common.lucene.search.XBooleanFilter; +import org.testng.annotations.AfterClass; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +import java.io.IOException; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.core.IsEqual.equalTo; + +/** + * Tests ported from Lucene. + */ +public class XBooleanFilterLuceneTests { + + private Directory directory; + private AtomicReader reader; + + @BeforeClass + public void setUp() throws Exception { + directory = new RAMDirectory(); + IndexWriter writer = new IndexWriter(directory, new IndexWriterConfig(Lucene.VERSION, new WhitespaceAnalyzer(Lucene.VERSION))); + + //Add series of docs with filterable fields : acces rights, prices, dates and "in-stock" flags + addDoc(writer, "admin guest", "010", "20040101", "Y"); + addDoc(writer, "guest", "020", "20040101", "Y"); + addDoc(writer, "guest", "020", "20050101", "Y"); + addDoc(writer, "admin", "020", "20050101", "Maybe"); + addDoc(writer, "admin guest", "030", "20050101", "N"); + writer.close(); + reader = new SlowCompositeReaderWrapper(DirectoryReader.open(directory)); + writer.close(); + } + + @AfterClass + public void tearDown() throws Exception { + reader.close(); + directory.close(); + } + + private void addDoc(IndexWriter writer, String accessRights, String price, String date, String inStock) throws IOException { + Document doc = new Document(); + doc.add(new TextField("accessRights", accessRights, Field.Store.YES)); + doc.add(new TextField("price", price, Field.Store.YES)); + doc.add(new TextField("date", date, Field.Store.YES)); + doc.add(new TextField("inStock", inStock, Field.Store.YES)); + writer.addDocument(doc); + } + + private Filter getRangeFilter(String field, String lowerPrice, String upperPrice) { + return TermRangeFilter.newStringRange(field, lowerPrice, upperPrice, true, true); + } + + private Filter getTermsFilter(String field, String text) { + return new TermsFilter(new Term(field, text)); + } + + private Filter getWrappedTermQuery(String field, String text) { + return new QueryWrapperFilter(new TermQuery(new Term(field, text))); + } + + private Filter getEmptyFilter() { + return new Filter() { + @Override + public DocIdSet getDocIdSet(AtomicReaderContext context, Bits acceptDocs) { + return new FixedBitSet(context.reader().maxDoc()); + } + }; + } + + private Filter getNullDISFilter() { + return new Filter() { + @Override + public DocIdSet getDocIdSet(AtomicReaderContext context, Bits acceptDocs) { + return null; + } + }; + } + + private Filter getNullDISIFilter() { + return new Filter() { + @Override + public DocIdSet getDocIdSet(AtomicReaderContext context, Bits acceptDocs) { + return new DocIdSet() { + @Override + public DocIdSetIterator iterator() { + return null; + } + + @Override + public boolean isCacheable() { + return true; + } + }; + } + }; + } + + private void tstFilterCard(String mes, int expected, Filter filt) throws Exception { + int actual = 0; + DocIdSet docIdSet = filt.getDocIdSet(reader.getContext(), reader.getLiveDocs()); + if (docIdSet != null) { + DocIdSetIterator disi = docIdSet.iterator(); + if (disi != null) { + while (disi.nextDoc() != DocIdSetIterator.NO_MORE_DOCS) { + actual++; + } + } + } + assertThat(mes, actual, equalTo(expected)); + } + + @Test + public void testShould() throws Exception { + XBooleanFilter booleanFilter = new XBooleanFilter(); + booleanFilter.add(getTermsFilter("price", "030"), BooleanClause.Occur.SHOULD); + tstFilterCard("Should retrieves only 1 doc", 1, booleanFilter); + + // same with a real DISI (no OpenBitSetIterator) + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getWrappedTermQuery("price", "030"), BooleanClause.Occur.SHOULD); + tstFilterCard("Should retrieves only 1 doc", 1, booleanFilter); + } + + @Test + public void testShoulds() throws Exception { + XBooleanFilter booleanFilter = new XBooleanFilter(); + booleanFilter.add(getRangeFilter("price", "010", "020"), BooleanClause.Occur.SHOULD); + booleanFilter.add(getRangeFilter("price", "020", "030"), BooleanClause.Occur.SHOULD); + tstFilterCard("Shoulds are Ored together", 5, booleanFilter); + } + + @Test + public void testShouldsAndMustNot() throws Exception { + XBooleanFilter booleanFilter = new XBooleanFilter(); + booleanFilter.add(getRangeFilter("price", "010", "020"), BooleanClause.Occur.SHOULD); + booleanFilter.add(getRangeFilter("price", "020", "030"), BooleanClause.Occur.SHOULD); + booleanFilter.add(getTermsFilter("inStock", "N"), BooleanClause.Occur.MUST_NOT); + tstFilterCard("Shoulds Ored but AndNot", 4, booleanFilter); + + booleanFilter.add(getTermsFilter("inStock", "Maybe"), BooleanClause.Occur.MUST_NOT); + tstFilterCard("Shoulds Ored but AndNots", 3, booleanFilter); + + // same with a real DISI (no OpenBitSetIterator) + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getRangeFilter("price", "010", "020"), BooleanClause.Occur.SHOULD); + booleanFilter.add(getRangeFilter("price", "020", "030"), BooleanClause.Occur.SHOULD); + booleanFilter.add(getWrappedTermQuery("inStock", "N"), BooleanClause.Occur.MUST_NOT); + tstFilterCard("Shoulds Ored but AndNot", 4, booleanFilter); + + booleanFilter.add(getWrappedTermQuery("inStock", "Maybe"), BooleanClause.Occur.MUST_NOT); + tstFilterCard("Shoulds Ored but AndNots", 3, booleanFilter); + } + + @Test + public void testShouldsAndMust() throws Exception { + XBooleanFilter booleanFilter = new XBooleanFilter(); + booleanFilter.add(getRangeFilter("price", "010", "020"), BooleanClause.Occur.SHOULD); + booleanFilter.add(getRangeFilter("price", "020", "030"), BooleanClause.Occur.SHOULD); + booleanFilter.add(getTermsFilter("accessRights", "admin"), BooleanClause.Occur.MUST); + tstFilterCard("Shoulds Ored but MUST", 3, booleanFilter); + + // same with a real DISI (no OpenBitSetIterator) + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getRangeFilter("price", "010", "020"), BooleanClause.Occur.SHOULD); + booleanFilter.add(getRangeFilter("price", "020", "030"), BooleanClause.Occur.SHOULD); + booleanFilter.add(getWrappedTermQuery("accessRights", "admin"), BooleanClause.Occur.MUST); + tstFilterCard("Shoulds Ored but MUST", 3, booleanFilter); + } + + @Test + public void testShouldsAndMusts() throws Exception { + XBooleanFilter booleanFilter = new XBooleanFilter(); + booleanFilter.add(getRangeFilter("price", "010", "020"), BooleanClause.Occur.SHOULD); + booleanFilter.add(getRangeFilter("price", "020", "030"), BooleanClause.Occur.SHOULD); + booleanFilter.add(getTermsFilter("accessRights", "admin"), BooleanClause.Occur.MUST); + booleanFilter.add(getRangeFilter("date", "20040101", "20041231"), BooleanClause.Occur.MUST); + tstFilterCard("Shoulds Ored but MUSTs ANDED", 1, booleanFilter); + } + + @Test + public void testShouldsAndMustsAndMustNot() throws Exception { + XBooleanFilter booleanFilter = new XBooleanFilter(); + booleanFilter.add(getRangeFilter("price", "030", "040"), BooleanClause.Occur.SHOULD); + booleanFilter.add(getTermsFilter("accessRights", "admin"), BooleanClause.Occur.MUST); + booleanFilter.add(getRangeFilter("date", "20050101", "20051231"), BooleanClause.Occur.MUST); + booleanFilter.add(getTermsFilter("inStock", "N"), BooleanClause.Occur.MUST_NOT); + tstFilterCard("Shoulds Ored but MUSTs ANDED and MustNot", 0, booleanFilter); + + // same with a real DISI (no OpenBitSetIterator) + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getRangeFilter("price", "030", "040"), BooleanClause.Occur.SHOULD); + booleanFilter.add(getWrappedTermQuery("accessRights", "admin"), BooleanClause.Occur.MUST); + booleanFilter.add(getRangeFilter("date", "20050101", "20051231"), BooleanClause.Occur.MUST); + booleanFilter.add(getWrappedTermQuery("inStock", "N"), BooleanClause.Occur.MUST_NOT); + tstFilterCard("Shoulds Ored but MUSTs ANDED and MustNot", 0, booleanFilter); + } + + @Test + public void testJustMust() throws Exception { + XBooleanFilter booleanFilter = new XBooleanFilter(); + booleanFilter.add(getTermsFilter("accessRights", "admin"), BooleanClause.Occur.MUST); + tstFilterCard("MUST", 3, booleanFilter); + + // same with a real DISI (no OpenBitSetIterator) + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getWrappedTermQuery("accessRights", "admin"), BooleanClause.Occur.MUST); + tstFilterCard("MUST", 3, booleanFilter); + } + + @Test + public void testJustMustNot() throws Exception { + XBooleanFilter booleanFilter = new XBooleanFilter(); + booleanFilter.add(getTermsFilter("inStock", "N"), BooleanClause.Occur.MUST_NOT); + tstFilterCard("MUST_NOT", 4, booleanFilter); + + // same with a real DISI (no OpenBitSetIterator) + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getWrappedTermQuery("inStock", "N"), BooleanClause.Occur.MUST_NOT); + tstFilterCard("MUST_NOT", 4, booleanFilter); + } + + @Test + public void testMustAndMustNot() throws Exception { + XBooleanFilter booleanFilter = new XBooleanFilter(); + booleanFilter.add(getTermsFilter("inStock", "N"), BooleanClause.Occur.MUST); + booleanFilter.add(getTermsFilter("price", "030"), BooleanClause.Occur.MUST_NOT); + tstFilterCard("MUST_NOT wins over MUST for same docs", 0, booleanFilter); + + // same with a real DISI (no OpenBitSetIterator) + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getWrappedTermQuery("inStock", "N"), BooleanClause.Occur.MUST); + booleanFilter.add(getWrappedTermQuery("price", "030"), BooleanClause.Occur.MUST_NOT); + tstFilterCard("MUST_NOT wins over MUST for same docs", 0, booleanFilter); + } + + @Test + public void testEmpty() throws Exception { + XBooleanFilter booleanFilter = new XBooleanFilter(); + tstFilterCard("empty XBooleanFilter returns no results", 0, booleanFilter); + } + + @Test + public void testCombinedNullDocIdSets() throws Exception { + XBooleanFilter booleanFilter = new XBooleanFilter(); + booleanFilter.add(getTermsFilter("price", "030"), BooleanClause.Occur.MUST); + booleanFilter.add(getNullDISFilter(), BooleanClause.Occur.MUST); + tstFilterCard("A MUST filter that returns a null DIS should never return documents", 0, booleanFilter); + + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getTermsFilter("price", "030"), BooleanClause.Occur.MUST); + booleanFilter.add(getNullDISIFilter(), BooleanClause.Occur.MUST); + tstFilterCard("A MUST filter that returns a null DISI should never return documents", 0, booleanFilter); + + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getTermsFilter("price", "030"), BooleanClause.Occur.SHOULD); + booleanFilter.add(getNullDISFilter(), BooleanClause.Occur.SHOULD); + tstFilterCard("A SHOULD filter that returns a null DIS should be invisible", 1, booleanFilter); + + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getTermsFilter("price", "030"), BooleanClause.Occur.SHOULD); + booleanFilter.add(getNullDISIFilter(), BooleanClause.Occur.SHOULD); + tstFilterCard("A SHOULD filter that returns a null DISI should be invisible", 1, booleanFilter); + + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getTermsFilter("price", "030"), BooleanClause.Occur.MUST); + booleanFilter.add(getNullDISFilter(), BooleanClause.Occur.MUST_NOT); + tstFilterCard("A MUST_NOT filter that returns a null DIS should be invisible", 1, booleanFilter); + + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getTermsFilter("price", "030"), BooleanClause.Occur.MUST); + booleanFilter.add(getNullDISIFilter(), BooleanClause.Occur.MUST_NOT); + tstFilterCard("A MUST_NOT filter that returns a null DISI should be invisible", 1, booleanFilter); + } + + @Test + public void testJustNullDocIdSets() throws Exception { + XBooleanFilter booleanFilter = new XBooleanFilter(); + booleanFilter.add(getNullDISFilter(), BooleanClause.Occur.MUST); + tstFilterCard("A MUST filter that returns a null DIS should never return documents", 0, booleanFilter); + + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getNullDISIFilter(), BooleanClause.Occur.MUST); + tstFilterCard("A MUST filter that returns a null DISI should never return documents", 0, booleanFilter); + + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getNullDISFilter(), BooleanClause.Occur.SHOULD); + tstFilterCard("A single SHOULD filter that returns a null DIS should never return documents", 0, booleanFilter); + + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getNullDISIFilter(), BooleanClause.Occur.SHOULD); + tstFilterCard("A single SHOULD filter that returns a null DISI should never return documents", 0, booleanFilter); + + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getNullDISFilter(), BooleanClause.Occur.MUST_NOT); + tstFilterCard("A single MUST_NOT filter that returns a null DIS should be invisible", 5, booleanFilter); + + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getNullDISIFilter(), BooleanClause.Occur.MUST_NOT); + tstFilterCard("A single MUST_NOT filter that returns a null DIS should be invisible", 5, booleanFilter); + } + + @Test + public void testNonMatchingShouldsAndMusts() throws Exception { + XBooleanFilter booleanFilter = new XBooleanFilter(); + booleanFilter.add(getEmptyFilter(), BooleanClause.Occur.SHOULD); + booleanFilter.add(getTermsFilter("accessRights", "admin"), BooleanClause.Occur.MUST); + tstFilterCard(">0 shoulds with no matches should return no docs", 0, booleanFilter); + + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getNullDISFilter(), BooleanClause.Occur.SHOULD); + booleanFilter.add(getTermsFilter("accessRights", "admin"), BooleanClause.Occur.MUST); + tstFilterCard(">0 shoulds with no matches should return no docs", 0, booleanFilter); + + booleanFilter = new XBooleanFilter(); + booleanFilter.add(getNullDISIFilter(), BooleanClause.Occur.SHOULD); + booleanFilter.add(getTermsFilter("accessRights", "admin"), BooleanClause.Occur.MUST); + tstFilterCard(">0 shoulds with no matches should return no docs", 0, booleanFilter); + } + + @Test + public void testToStringOfBooleanFilterContainingTermsFilter() { + XBooleanFilter booleanFilter = new XBooleanFilter(); + booleanFilter.add(getTermsFilter("inStock", "N"), BooleanClause.Occur.MUST); + booleanFilter.add(getTermsFilter("isFragile", "Y"), BooleanClause.Occur.MUST); + + assertThat("BooleanFilter(+inStock:N +isFragile:Y)", equalTo(booleanFilter.toString())); + } + + @Test + public void testToStringOfWrappedBooleanFilters() { + XBooleanFilter orFilter = new XBooleanFilter(); + + XBooleanFilter stockFilter = new XBooleanFilter(); + stockFilter.add(new FilterClause(getTermsFilter("inStock", "Y"), BooleanClause.Occur.MUST)); + stockFilter.add(new FilterClause(getTermsFilter("barCode", "12345678"), BooleanClause.Occur.MUST)); + + orFilter.add(new FilterClause(stockFilter, BooleanClause.Occur.SHOULD)); + + XBooleanFilter productPropertyFilter = new XBooleanFilter(); + productPropertyFilter.add(new FilterClause(getTermsFilter("isHeavy", "N"), BooleanClause.Occur.MUST)); + productPropertyFilter.add(new FilterClause(getTermsFilter("isDamaged", "Y"), BooleanClause.Occur.MUST)); + + orFilter.add(new FilterClause(productPropertyFilter, BooleanClause.Occur.SHOULD)); + + XBooleanFilter composedFilter = new XBooleanFilter(); + composedFilter.add(new FilterClause(orFilter, BooleanClause.Occur.MUST)); + + assertThat( + "BooleanFilter(+BooleanFilter(BooleanFilter(+inStock:Y +barCode:12345678) BooleanFilter(+isHeavy:N +isDamaged:Y)))", + equalTo(composedFilter.toString()) + ); + } + +} diff --git a/src/test/java/org/elasticsearch/test/unit/common/lucene/search/XBooleanFilterTests.java b/src/test/java/org/elasticsearch/test/unit/common/lucene/search/XBooleanFilterTests.java index c4521071961..ba8d63d83e9 100644 --- a/src/test/java/org/elasticsearch/test/unit/common/lucene/search/XBooleanFilterTests.java +++ b/src/test/java/org/elasticsearch/test/unit/common/lucene/search/XBooleanFilterTests.java @@ -7,6 +7,7 @@ import org.apache.lucene.document.StringField; import org.apache.lucene.index.*; import org.apache.lucene.queries.FilterClause; import org.apache.lucene.search.BooleanClause; +import org.apache.lucene.search.DocIdSet; import org.apache.lucene.search.FieldCacheTermsFilter; import org.apache.lucene.search.Filter; import org.apache.lucene.store.Directory; @@ -36,9 +37,9 @@ public class XBooleanFilterTests { @BeforeClass public void setup() throws Exception { char[][] documentMatrix = new char[][] { - {'a', 'b', 'c', 'd'}, - {'a', 'b', 'c', 'd'}, - {'a', 'a', 'a', 'a'} + {'a', 'b', 'c', 'd', 'v'}, + {'a', 'b', 'c', 'd', 'z'}, + {'a', 'a', 'a', 'a', 'x'} }; List documents = new ArrayList(documentMatrix.length); @@ -218,6 +219,131 @@ public class XBooleanFilterTests { } } + @Test + public void testOnlyShouldClauses() throws Exception { + List booleanFilters = new ArrayList(); + // 2 slow filters + // This case caused: https://github.com/elasticsearch/elasticsearch/issues/2826 + booleanFilters.add(createBooleanFilter( + newFilterClause(1, 'a', SHOULD, true), + newFilterClause(1, 'b', SHOULD, true) + )); + // 2 fast filters + booleanFilters.add(createBooleanFilter( + newFilterClause(1, 'a', SHOULD, false), + newFilterClause(1, 'b', SHOULD, false) + )); + // 1 fast filters, 1 slow filter + booleanFilters.add(createBooleanFilter( + newFilterClause(1, 'a', SHOULD, true), + newFilterClause(1, 'b', SHOULD, false) + )); + + for (XBooleanFilter booleanFilter : booleanFilters) { + FixedBitSet result = new FixedBitSet(reader.maxDoc()); + result.or(booleanFilter.getDocIdSet(reader.getContext(), reader.getLiveDocs()).iterator()); + assertThat(result.cardinality(), equalTo(3)); + assertThat(result.get(0), equalTo(true)); + assertThat(result.get(1), equalTo(true)); + assertThat(result.get(2), equalTo(true)); + } + } + + @Test + public void testOnlyMustClauses() throws Exception { + List booleanFilters = new ArrayList(); + // Slow filters + booleanFilters.add(createBooleanFilter( + newFilterClause(3, 'd', MUST, true), + newFilterClause(3, 'd', MUST, true) + )); + // 2 fast filters + booleanFilters.add(createBooleanFilter( + newFilterClause(3, 'd', MUST, false), + newFilterClause(3, 'd', MUST, false) + )); + // 1 fast filters, 1 slow filter + booleanFilters.add(createBooleanFilter( + newFilterClause(3, 'd', MUST, true), + newFilterClause(3, 'd', MUST, false) + )); + for (XBooleanFilter booleanFilter : booleanFilters) { + FixedBitSet result = new FixedBitSet(reader.maxDoc()); + result.or(booleanFilter.getDocIdSet(reader.getContext(), reader.getLiveDocs()).iterator()); + assertThat(result.cardinality(), equalTo(2)); + assertThat(result.get(0), equalTo(true)); + assertThat(result.get(1), equalTo(true)); + assertThat(result.get(2), equalTo(false)); + } + } + + @Test + public void testOnlyMustNotClauses() throws Exception { + List booleanFilters = new ArrayList(); + // Slow filters + booleanFilters.add(createBooleanFilter( + newFilterClause(1, 'a', MUST_NOT, true), + newFilterClause(1, 'a', MUST_NOT, true) + )); + // 2 fast filters + booleanFilters.add(createBooleanFilter( + newFilterClause(1, 'a', MUST_NOT, false), + newFilterClause(1, 'a', MUST_NOT, false) + )); + // 1 fast filters, 1 slow filter + booleanFilters.add(createBooleanFilter( + newFilterClause(1, 'a', MUST_NOT, true), + newFilterClause(1, 'a', MUST_NOT, false) + )); + for (XBooleanFilter booleanFilter : booleanFilters) { + FixedBitSet result = new FixedBitSet(reader.maxDoc()); + result.or(booleanFilter.getDocIdSet(reader.getContext(), reader.getLiveDocs()).iterator()); + assertThat(result.cardinality(), equalTo(2)); + assertThat(result.get(0), equalTo(true)); + assertThat(result.get(1), equalTo(true)); + assertThat(result.get(2), equalTo(false)); + } + } + + @Test + public void testNonMatchingSlowShouldWithMatchingMust() throws Exception { + XBooleanFilter booleanFilter = createBooleanFilter( + newFilterClause(0, 'a', MUST, false), + newFilterClause(0, 'b', SHOULD, true) + ); + + DocIdSet docIdSet = booleanFilter.getDocIdSet(reader.getContext(), reader.getLiveDocs()); + assertThat(docIdSet, equalTo(null)); + } + + @Test + public void testSlowShouldClause_atLeastOneShouldMustMatch() throws Exception { + XBooleanFilter booleanFilter = createBooleanFilter( + newFilterClause(0, 'a', MUST, false), + newFilterClause(1, 'a', SHOULD, true) + ); + + FixedBitSet result = new FixedBitSet(reader.maxDoc()); + result.or(booleanFilter.getDocIdSet(reader.getContext(), reader.getLiveDocs()).iterator()); + assertThat(result.cardinality(), equalTo(1)); + assertThat(result.get(0), equalTo(false)); + assertThat(result.get(1), equalTo(false)); + assertThat(result.get(2), equalTo(true)); + + booleanFilter = createBooleanFilter( + newFilterClause(0, 'a', MUST, false), + newFilterClause(1, 'a', SHOULD, true), + newFilterClause(4, 'z', SHOULD, true) + ); + + result = new FixedBitSet(reader.maxDoc()); + result.or(booleanFilter.getDocIdSet(reader.getContext(), reader.getLiveDocs()).iterator()); + assertThat(result.cardinality(), equalTo(2)); + assertThat(result.get(0), equalTo(false)); + assertThat(result.get(1), equalTo(true)); + assertThat(result.get(2), equalTo(true)); + } + private static FilterClause newFilterClause(int field, char character, BooleanClause.Occur occur, boolean slowerBitsBackedFilter) { Filter filter; if (slowerBitsBackedFilter) {