Skip to content

Commit

Permalink
bool query: parser should return match_all in case there are no clauses
Browse files Browse the repository at this point in the history
This also fixes has_parent filters with a nested empty bool filter
(see test SimpleChildQuerySearchTests#test6722, the test should actually expect
either 0 results when searching for has_parent "test" or one result when
search for has_parent "foo")

closes #7240
closes #7347
  • Loading branch information
brwe authored and areek committed Sep 8, 2014
1 parent 47f0a78 commit a9bd0d9
Show file tree
Hide file tree
Showing 9 changed files with 193 additions and 6 deletions.
Expand Up @@ -21,6 +21,7 @@

import org.apache.lucene.search.BooleanClause;
import org.apache.lucene.search.BooleanQuery;
import org.apache.lucene.search.MatchAllDocsQuery;
import org.apache.lucene.search.Query;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.lucene.search.Queries;
Expand Down Expand Up @@ -131,7 +132,7 @@ public Query parse(QueryParseContext parseContext) throws IOException, QueryPars
}

if (clauses.isEmpty()) {
return null;
return new MatchAllDocsQuery();
}

BooleanQuery booleanQuery = new BooleanQuery(disableCoord);
Expand Down
Expand Up @@ -45,15 +45,20 @@
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.DistanceUnit;
import org.elasticsearch.common.unit.Fuzziness;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.cache.filter.support.CacheKeyFilter;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.mapper.core.NumberFieldMapper;
import org.elasticsearch.index.search.NumericRangeFieldDataFilter;
import org.elasticsearch.index.search.child.CustomQueryWrappingFilter;
import org.elasticsearch.index.search.child.ParentConstantScoreQuery;
import org.elasticsearch.index.search.geo.GeoDistanceFilter;
import org.elasticsearch.index.search.geo.GeoPolygonFilter;
import org.elasticsearch.index.search.geo.InMemoryGeoBoundingBoxFilter;
import org.elasticsearch.index.search.morelikethis.MoreLikeThisFetchService;
import org.elasticsearch.index.service.IndexService;
import org.elasticsearch.search.internal.SearchContext;
import org.elasticsearch.test.ElasticsearchSingleNodeTest;
import org.hamcrest.Matchers;
import org.junit.Before;
Expand All @@ -67,6 +72,7 @@

import static org.elasticsearch.common.io.Streams.copyToBytesFromClasspath;
import static org.elasticsearch.common.io.Streams.copyToStringFromClasspath;
import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
import static org.elasticsearch.index.query.FilterBuilders.*;
import static org.elasticsearch.index.query.QueryBuilders.*;
import static org.elasticsearch.index.query.RegexpFlag.*;
Expand Down Expand Up @@ -2289,5 +2295,42 @@ public void testMatchWithoutFuzzyTranspositions() throws Exception {
assertThat( ((FuzzyQuery) parsedQuery).getTranspositions(), equalTo(false));
}

// https://github.com/elasticsearch/elasticsearch/issues/7240
@Test
public void testEmptyBooleanQuery() throws Exception {
IndexQueryParserService queryParser = queryParser();
String query = jsonBuilder().startObject().startObject("bool").endObject().endObject().string();
Query parsedQuery = queryParser.parse(query).query();
assertThat(parsedQuery, instanceOf(MatchAllDocsQuery.class));
}

// https://github.com/elasticsearch/elasticsearch/issues/7240
@Test
public void testEmptyBooleanQueryInsideFQuery() throws Exception {
IndexQueryParserService queryParser = queryParser();
String query = copyToStringFromClasspath("/org/elasticsearch/index/query/fquery-with-empty-bool-query.json");
XContentParser parser = XContentHelper.createParser(new BytesArray(query));
ParsedFilter parsedQuery = queryParser.parseInnerFilter(parser);
assertThat(parsedQuery.filter(), instanceOf(QueryWrapperFilter.class));
assertThat(((QueryWrapperFilter) parsedQuery.filter()).getQuery(), instanceOf(XFilteredQuery.class));
assertThat(((XFilteredQuery) ((QueryWrapperFilter) parsedQuery.filter()).getQuery()).getFilter(), instanceOf(TermFilter.class));
TermFilter filter = (TermFilter) ((XFilteredQuery) ((QueryWrapperFilter) parsedQuery.filter()).getQuery()).getFilter();
assertThat(filter.getTerm().toString(), equalTo("text:apache"));
}

// https://github.com/elasticsearch/elasticsearch/issues/6722
public void testEmptyBoolSubClausesIsMatchAll() throws ElasticsearchException, IOException {
String query = copyToStringFromClasspath("/org/elasticsearch/index/query/bool-query-with-empty-clauses-for-parsing.json");
IndexService indexService = createIndex("testidx", client().admin().indices().prepareCreate("testidx")
.addMapping("foo")
.addMapping("test", "_parent", "type=foo"));
SearchContext.setCurrent(createSearchContext(indexService));
IndexQueryParserService queryParser = indexService.queryParserService();
Query parsedQuery = queryParser.parse(query).query();
assertThat(parsedQuery, instanceOf(XConstantScoreQuery.class));
assertThat(((XConstantScoreQuery) parsedQuery).getFilter(), instanceOf(CustomQueryWrappingFilter.class));
assertThat(((CustomQueryWrappingFilter) ((XConstantScoreQuery) parsedQuery).getFilter()).getQuery(), instanceOf(ParentConstantScoreQuery.class));
assertThat(((CustomQueryWrappingFilter) ((XConstantScoreQuery) parsedQuery).getFilter()).getQuery().toString(), equalTo("parent_filter[foo](*:*)"));
SearchContext.removeCurrent();
}
}
@@ -0,0 +1,17 @@
{
"filtered": {
"filter": {
"has_parent": {
"type": "foo",
"query": {
"bool": {
"must": [],
"must_not": [],
"should": []
}
}
},
"query": []
}
}
}
@@ -0,0 +1,16 @@
{
"fquery": {
"query": {
"filtered": {
"query": {
"bool": {}
},
"filter": {
"term": {
"text": "apache"
}
}
}
}
}
}
@@ -0,0 +1,56 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch 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.
*/
package org.elasticsearch.search.aggregations.bucket;

import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.search.aggregations.bucket.filter.Filter;
import org.elasticsearch.search.aggregations.bucket.terms.StringTerms;
import org.elasticsearch.test.ElasticsearchIntegrationTest;
import org.junit.Test;

import java.io.IOException;

import static org.elasticsearch.common.io.Streams.copyToStringFromClasspath;
import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.instanceOf;


public class DedicatedAggregationTests extends ElasticsearchIntegrationTest {

// https://github.com/elasticsearch/elasticsearch/issues/7240
@Test
public void testEmptyBoolIsMatchAll() throws IOException {
String query = copyToStringFromClasspath("/org/elasticsearch/search/aggregations/bucket/agg-filter-with-empty-bool.json");
createIndex("testidx");
index("testidx", "apache", "1", "field", "text");
index("testidx", "nginx", "2", "field", "text");
refresh();
ensureGreen("testidx");
SearchResponse searchResponse = client().prepareSearch("testidx").setQuery(matchAllQuery()).get();
assertThat(searchResponse.getHits().getTotalHits(), equalTo(2l));
searchResponse = client().prepareSearch("testidx").setSource(query).get();
assertSearchResponse(searchResponse);
assertThat(searchResponse.getAggregations().getAsMap().get("issue7240"), instanceOf(Filter.class));
Filter filterAgg = (Filter) searchResponse.getAggregations().getAsMap().get("issue7240");
assertThat(filterAgg.getAggregations().getAsMap().get("terms"), instanceOf(StringTerms.class));
assertThat(((StringTerms) filterAgg.getAggregations().getAsMap().get("terms")).getBuckets().get(0).getDocCount(), equalTo(1l));
}
}
@@ -0,0 +1,33 @@
{
"aggs": {
"issue7240": {
"aggs": {
"terms": {
"terms": {
"field": "field"
}
}
},
"filter": {
"fquery": {
"query": {
"filtered": {
"query": {
"bool": {}
},
"filter": {
"fquery": {
"query": {
"query_string": {
"query": "_type:apache"
}
}
}
}
}
}
}
}
}
}
}
Expand Up @@ -59,6 +59,7 @@

import static com.google.common.collect.Maps.newHashMap;
import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF_REPLICAS;
import static org.elasticsearch.common.io.Streams.copyToStringFromClasspath;
import static org.elasticsearch.common.settings.ImmutableSettings.builder;
import static org.elasticsearch.common.settings.ImmutableSettings.settingsBuilder;
import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
Expand Down Expand Up @@ -138,12 +139,13 @@ public void test6722() throws ElasticsearchException, IOException {

// index simple data
client().prepareIndex("test", "foo", "1").setSource("foo", 1).get();
client().prepareIndex("test", "test").setSource("foo", 1).setParent("1").get();
client().prepareIndex("test", "test", "2").setSource("foo", 1).setParent("1").get();
refresh();

SearchResponse searchResponse = client().prepareSearch("test").setSource("{\"query\":{\"filtered\":{\"filter\":{\"has_parent\":{\"type\":\"test\",\"query\":{\"bool\":{\"must\":[],\"must_not\":[],\"should\":[]}}},\"query\":[]}}}}").get();
String query = copyToStringFromClasspath("/org/elasticsearch/search/child/bool-query-with-empty-clauses.json");
SearchResponse searchResponse = client().prepareSearch("test").setSource(query).get();
assertNoFailures(searchResponse);
assertThat(searchResponse.getHits().totalHits(), equalTo(2l));
assertThat(searchResponse.getHits().totalHits(), equalTo(1l));
assertThat(searchResponse.getHits().getAt(0).getId(), equalTo("2"));
}

@Test
Expand Down
@@ -0,0 +1,19 @@
{
"query": {
"filtered": {
"filter": {
"has_parent": {
"type": "foo",
"query": {
"bool": {
"must": [],
"must_not": [],
"should": []
}
}
},
"query": []
}
}
}
}
Expand Up @@ -160,7 +160,7 @@ protected static IndexService createIndex(String index, Settings settings, Strin
return createIndex(index, createIndexRequestBuilder);
}

private static IndexService createIndex(String index, CreateIndexRequestBuilder createIndexRequestBuilder) {
protected static IndexService createIndex(String index, CreateIndexRequestBuilder createIndexRequestBuilder) {
assertAcked(createIndexRequestBuilder.get());
// Wait for the index to be allocated so that cluster state updates don't override
// changes that would have been done locally
Expand Down

0 comments on commit a9bd0d9

Please sign in to comment.