Fixed issue where 'fast' should filter can make documents that didn't match the must or must_not clause a match again. Relates to #2979

src/main/java/org/elasticsearch/common/lucene/search/XBooleanFilter.java

@@ -31,9 +31,7 @@ import org.elasticsearch.common.lucene.docset.DocIdSets;
 import org.elasticsearch.common.lucene.docset.NotDocIdSet;
 
 import java.io.IOException;
-import java.util.ArrayList;
+import java.util.*;
-import java.util.Iterator;
-import java.util.List;
 
 /**
  * Similar to {@link org.apache.lucene.queries.BooleanFilter}.
@@ -80,6 +78,7 @@ public class XBooleanFilter extends Filter implements Iterable<FilterClause> {
         boolean hasNonEmptyShouldClause = false;
         boolean hasMustClauses = false;
         boolean hasMustNotClauses = false;
+        boolean mustOrMustNotBeforeShould = false;
         for (int i = 0; i < clauses.size(); i++) {
             FilterClause clause = clauses.get(i);
             DocIdSet set = clause.getFilter().getDocIdSet(context, acceptDocs);
@@ -89,6 +88,9 @@ public class XBooleanFilter extends Filter implements Iterable<FilterClause> {
                     return null;
                 }
             } else if (clause.getOccur() == Occur.SHOULD) {
+                if (!hasShouldClauses && (hasMustClauses || hasMustNotClauses)) {
+                    mustOrMustNotBeforeShould = true;
+                }
                 hasShouldClauses = true;
                 if (DocIdSets.isEmpty(set)) {
                     continue;
@@ -113,6 +115,32 @@ public class XBooleanFilter extends Filter implements Iterable<FilterClause> {
             return null;
         }
 
+        if (mustOrMustNotBeforeShould) {
+            // Sort the clause only once if we encounter a should before a must or must_not clause
+            Collections.sort(clauses, new Comparator<FilterClause>() {
+                @Override
+                public int compare(FilterClause o1, FilterClause o2) {
+                    if (o1.getOccur() != o2.getOccur()) {
+                        return o1.getOccur() == Occur.SHOULD ? -1 : 1;
+                    } else {
+                        return 0;
+                    }
+                }
+            });
+
+            // Because we sorted the clause we also need to sort the result clauses
+            Collections.sort(results, new Comparator<ResultClause>() {
+                @Override
+                public int compare(ResultClause o1, ResultClause o2) {
+                    if (o1.clause.getOccur() != o2.clause.getOccur()) {
+                        return o1.clause.getOccur() == Occur.SHOULD ? -1 : 1;
+                    } else {
+                        return 0;
+                    }
+                }
+            });
+        }
+
         // now, go over the clauses and apply the "fast" ones...
         hasNonEmptyShouldClause = false;
         boolean hasBits = false;

src/test/java/org/elasticsearch/test/integration/search/query/SimpleQueryTests.java

@@ -20,7 +20,6 @@
 package org.elasticsearch.test.integration.search.query;
 
 import org.elasticsearch.ElasticSearchException;
-import org.elasticsearch.action.admin.indices.create.CreateIndexResponse;
 import org.elasticsearch.action.search.SearchPhaseExecutionException;
 import org.elasticsearch.action.search.SearchResponse;
 import org.elasticsearch.action.search.SearchType;
@@ -46,7 +45,6 @@ import static org.elasticsearch.index.query.FilterBuilders.*;
 import static org.elasticsearch.index.query.QueryBuilders.*;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.anyOf;
-import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.equalTo;
 import static org.testng.Assert.assertTrue;
 import static org.testng.Assert.fail;
@@ -1203,21 +1201,25 @@ public class SimpleQueryTests extends AbstractNodesTests {
                 .execute().actionGet();
 
         client.prepareIndex("test", "type1", "1").setSource(jsonBuilder().startObject()
+                .field("field1", "test1")
                 .field("num_long", 1)
                 .endObject())
                 .execute().actionGet();
 
         client.prepareIndex("test", "type1", "2").setSource(jsonBuilder().startObject()
+                .field("field1", "test1")
                 .field("num_long", 2)
                 .endObject())
                 .execute().actionGet();
 
         client.prepareIndex("test", "type1", "3").setSource(jsonBuilder().startObject()
+                .field("field1", "test2")
                 .field("num_long", 3)
                 .endObject())
                 .execute().actionGet();
 
         client.prepareIndex("test", "type1", "4").setSource(jsonBuilder().startObject()
+                .field("field1", "test2")
                 .field("num_long", 4)
                 .endObject())
                 .execute().actionGet();
@@ -1238,6 +1240,16 @@ public class SimpleQueryTests extends AbstractNodesTests {
         ).execute().actionGet();
 
         assertThat(response.getHits().totalHits(), equalTo(4l));
+
+        // This made #2979 fail!
+        response = client.prepareSearch("test").setFilter(
+                FilterBuilders.boolFilter()
+                        .must(FilterBuilders.termFilter("field1", "test1"))
+                        .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(2l));
     }
     
     @Test // see #2926