Adressing review comments

This commit is contained in:
Christoph Büscher 2016-06-01 19:48:22 +02:00
parent e2b6dbc020
commit 9067407cdd
9 changed files with 59 additions and 47 deletions

View File

@ -91,7 +91,7 @@ public class ConstantScoreQueryBuilder extends AbstractQueryBuilder<ConstantScor
public static Optional<ConstantScoreQueryBuilder> fromXContent(QueryParseContext parseContext) throws IOException { public static Optional<ConstantScoreQueryBuilder> fromXContent(QueryParseContext parseContext) throws IOException {
XContentParser parser = parseContext.parser(); XContentParser parser = parseContext.parser();
Optional<QueryBuilder> query = Optional.empty(); Optional<QueryBuilder> query = null;
boolean queryFound = false; boolean queryFound = false;
String queryName = null; String queryName = null;
float boost = AbstractQueryBuilder.DEFAULT_BOOST; float boost = AbstractQueryBuilder.DEFAULT_BOOST;

View File

@ -25,6 +25,7 @@ import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.ParsingException;
import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.lucene.search.Queries;
import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentParser;
@ -168,7 +169,7 @@ public class DisMaxQueryBuilder extends AbstractQueryBuilder<DisMaxQueryBuilder>
} }
} }
if (!queriesFound || queries.isEmpty()) { if (!queriesFound) {
throw new ParsingException(parser.getTokenLocation(), "[dis_max] requires 'queries' field with at least one clause"); throw new ParsingException(parser.getTokenLocation(), "[dis_max] requires 'queries' field with at least one clause");
} }
@ -186,6 +187,10 @@ public class DisMaxQueryBuilder extends AbstractQueryBuilder<DisMaxQueryBuilder>
protected Query doToQuery(QueryShardContext context) throws IOException { protected Query doToQuery(QueryShardContext context) throws IOException {
// return null if there are no queries at all // return null if there are no queries at all
Collection<Query> luceneQueries = toQueries(queries, context); Collection<Query> luceneQueries = toQueries(queries, context);
if (luceneQueries.isEmpty()) {
return Queries.newMatchNoDocsQuery("no clauses for dismax query.");
}
return new DisjunctionMaxQuery(luceneQueries, tieBreaker); return new DisjunctionMaxQuery(luceneQueries, tieBreaker);
} }

View File

@ -611,7 +611,7 @@ public final class SearchSourceBuilder extends ToXContentToBytes implements Writ
/** /**
* Add an aggregation to perform as part of the search. * Add an aggregation to perform as part of the search.
*/ */
public SearchSourceBuilder aggregation(PipelineAggregatorBuilder<?> aggregation) { public SearchSourceBuilder aggregation(PipelineAggregatorBuilder aggregation) {
if (aggregations == null) { if (aggregations == null) {
aggregations = AggregatorFactories.builder(); aggregations = AggregatorFactories.builder();
} }

View File

@ -72,6 +72,9 @@ public class QueryRescorerBuilder extends RescoreBuilder<QueryRescorerBuilder> {
* @param builder the query builder to build the rescore query from * @param builder the query builder to build the rescore query from
*/ */
public QueryRescorerBuilder(QueryBuilder builder) { public QueryRescorerBuilder(QueryBuilder builder) {
if (builder == null) {
throw new IllegalArgumentException("rescore_query cannot be null");
}
this.queryBuilder = builder; this.queryBuilder = builder;
} }

View File

@ -25,7 +25,7 @@ import org.apache.lucene.search.DisjunctionMaxQuery;
import org.apache.lucene.search.PrefixQuery; import org.apache.lucene.search.PrefixQuery;
import org.apache.lucene.search.Query; import org.apache.lucene.search.Query;
import org.elasticsearch.common.ParseFieldMatcher; import org.elasticsearch.common.ParseFieldMatcher;
import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.lucene.search.MatchNoDocsQuery;
import org.elasticsearch.test.AbstractQueryTestCase; import org.elasticsearch.test.AbstractQueryTestCase;
import java.io.IOException; import java.io.IOException;
@ -35,7 +35,6 @@ import java.util.Iterator;
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
import static org.hamcrest.CoreMatchers.nullValue;
import static org.hamcrest.Matchers.closeTo; import static org.hamcrest.Matchers.closeTo;
import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.instanceOf;
@ -60,9 +59,6 @@ public class DisMaxQueryBuilderTests extends AbstractQueryTestCase<DisMaxQueryBu
@Override @Override
protected void doAssertLuceneQuery(DisMaxQueryBuilder queryBuilder, Query query, QueryShardContext context) throws IOException { protected void doAssertLuceneQuery(DisMaxQueryBuilder queryBuilder, Query query, QueryShardContext context) throws IOException {
Collection<Query> queries = AbstractQueryBuilder.toQueries(queryBuilder.innerQueries(), context); Collection<Query> queries = AbstractQueryBuilder.toQueries(queryBuilder.innerQueries(), context);
if (queries.isEmpty()) {
assertThat(query, nullValue());
} else {
assertThat(query, instanceOf(DisjunctionMaxQuery.class)); assertThat(query, instanceOf(DisjunctionMaxQuery.class));
DisjunctionMaxQuery disjunctionMaxQuery = (DisjunctionMaxQuery) query; DisjunctionMaxQuery disjunctionMaxQuery = (DisjunctionMaxQuery) query;
assertThat(disjunctionMaxQuery.getTieBreakerMultiplier(), equalTo(queryBuilder.tieBreaker())); assertThat(disjunctionMaxQuery.getTieBreakerMultiplier(), equalTo(queryBuilder.tieBreaker()));
@ -72,7 +68,6 @@ public class DisMaxQueryBuilderTests extends AbstractQueryTestCase<DisMaxQueryBu
assertThat(disjunctionMaxQuery.getDisjuncts().get(i), equalTo(queryIterator.next())); assertThat(disjunctionMaxQuery.getDisjuncts().get(i), equalTo(queryIterator.next()));
} }
} }
}
@Override @Override
protected Map<String, DisMaxQueryBuilder> getAlternateVersions() { protected Map<String, DisMaxQueryBuilder> getAlternateVersions() {
@ -90,7 +85,7 @@ public class DisMaxQueryBuilderTests extends AbstractQueryTestCase<DisMaxQueryBu
} }
/** /**
* Test with empty inner query body, this should be ignored upstream. * Test with empty inner query body, this should be converted to a {@link MatchNoDocsQuery}.
* To test this, we use inner {@link ConstantScoreQueryBuilder} with empty inner filter. * To test this, we use inner {@link ConstantScoreQueryBuilder} with empty inner filter.
*/ */
public void testInnerQueryEmptyException() throws IOException { public void testInnerQueryEmptyException() throws IOException {
@ -98,8 +93,11 @@ public class DisMaxQueryBuilderTests extends AbstractQueryTestCase<DisMaxQueryBu
+ " { \"queries\" : [ {\"" + ConstantScoreQueryBuilder.NAME + "\" : { \"filter\" : { } } } ] " + " { \"queries\" : [ {\"" + ConstantScoreQueryBuilder.NAME + "\" : { \"filter\" : { } } } ] "
+ " }" + " }"
+ " }"; + " }";
ParsingException ex = expectThrows(ParsingException.class, () -> parseQuery(queryString, ParseFieldMatcher.EMPTY)); QueryBuilder queryBuilder = parseQuery(queryString, ParseFieldMatcher.EMPTY);
assertEquals("[dis_max] requires 'queries' field with at least one clause", ex.getMessage()); QueryShardContext context = createShardContext();
Query luceneQuery = queryBuilder.toQuery(context);
assertThat(luceneQuery, instanceOf(MatchNoDocsQuery.class));
assertThat(((MatchNoDocsQuery) luceneQuery).toString(), equalTo("MatchNoDocsQuery[\"no clauses for dismax query.\"]"));
} }
public void testIllegalArguments() { public void testIllegalArguments() {

View File

@ -132,7 +132,7 @@ public class FilterIT extends ESIntegTestCase {
String emtpyFilterBody = "{ }"; String emtpyFilterBody = "{ }";
XContentParser parser = XContentFactory.xContent(emtpyFilterBody).createParser(emtpyFilterBody); XContentParser parser = XContentFactory.xContent(emtpyFilterBody).createParser(emtpyFilterBody);
QueryParseContext parseContext = new QueryParseContext(new IndicesQueriesRegistry(), parser, ParseFieldMatcher.EMPTY); QueryParseContext parseContext = new QueryParseContext(new IndicesQueriesRegistry(), parser, ParseFieldMatcher.EMPTY);
AggregationBuilder<?> filterAgg = FilterAggregationBuilder.parse("tag1", parseContext); AggregationBuilder filterAgg = FilterAggregationBuilder.parse("tag1", parseContext);
SearchResponse response = client().prepareSearch("idx").addAggregation(filterAgg).execute().actionGet(); SearchResponse response = client().prepareSearch("idx").addAggregation(filterAgg).execute().actionGet();

View File

@ -217,7 +217,7 @@ public class FiltersIT extends ESIntegTestCase {
XContentParser parser = XContentFactory.xContent(emtpyFilterBody).createParser(emtpyFilterBody); XContentParser parser = XContentFactory.xContent(emtpyFilterBody).createParser(emtpyFilterBody);
parser.nextToken(); parser.nextToken();
QueryParseContext parseContext = new QueryParseContext(new IndicesQueriesRegistry(), parser, ParseFieldMatcher.EMPTY); QueryParseContext parseContext = new QueryParseContext(new IndicesQueriesRegistry(), parser, ParseFieldMatcher.EMPTY);
AggregationBuilder<?> filtersAgg = FiltersAggregationBuilder.parse("tag1", parseContext); AggregationBuilder filtersAgg = FiltersAggregationBuilder.parse("tag1", parseContext);
SearchResponse response = client().prepareSearch("idx").addAggregation(filtersAgg).execute().actionGet(); SearchResponse response = client().prepareSearch("idx").addAggregation(filtersAgg).execute().actionGet();
@ -234,7 +234,7 @@ public class FiltersIT extends ESIntegTestCase {
XContentParser parser = XContentFactory.xContent(emtpyFilterBody).createParser(emtpyFilterBody); XContentParser parser = XContentFactory.xContent(emtpyFilterBody).createParser(emtpyFilterBody);
parser.nextToken(); parser.nextToken();
QueryParseContext parseContext = new QueryParseContext(new IndicesQueriesRegistry(), parser, ParseFieldMatcher.EMPTY); QueryParseContext parseContext = new QueryParseContext(new IndicesQueriesRegistry(), parser, ParseFieldMatcher.EMPTY);
AggregationBuilder<?> filtersAgg = FiltersAggregationBuilder.parse("tag1", parseContext); AggregationBuilder filtersAgg = FiltersAggregationBuilder.parse("tag1", parseContext);
SearchResponse response = client().prepareSearch("idx").addAggregation(filtersAgg) SearchResponse response = client().prepareSearch("idx").addAggregation(filtersAgg)
.execute().actionGet(); .execute().actionGet();

View File

@ -54,6 +54,7 @@ import org.junit.AfterClass;
import org.junit.BeforeClass; import org.junit.BeforeClass;
import java.io.IOException; import java.io.IOException;
import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.not;
@ -180,6 +181,11 @@ public class QueryRescoreBuilderTests extends ESTestCase {
} }
} }
public void testRescoreQueryNull() throws IOException {
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> new QueryRescorerBuilder((QueryBuilder) null));
assertEquals("rescore_query cannot be null", e.getMessage());
}
/** /**
* test parsing exceptions for incorrect rescorer syntax * test parsing exceptions for incorrect rescorer syntax
*/ */

View File

@ -19,26 +19,9 @@
package org.elasticsearch.test; package org.elasticsearch.test;
import static org.hamcrest.Matchers.containsString; import com.carrotsearch.randomizedtesting.generators.CodepointSetGenerator;
import static org.hamcrest.Matchers.either; import com.fasterxml.jackson.core.JsonParseException;
import static org.hamcrest.Matchers.equalTo; import com.fasterxml.jackson.core.io.JsonStringEncoder;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.not;
import java.io.Closeable;
import java.io.IOException;
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import org.apache.lucene.search.BoostQuery; import org.apache.lucene.search.BoostQuery;
import org.apache.lucene.search.Query; import org.apache.lucene.search.Query;
@ -132,9 +115,26 @@ import org.junit.After;
import org.junit.AfterClass; import org.junit.AfterClass;
import org.junit.Before; import org.junit.Before;
import com.carrotsearch.randomizedtesting.generators.CodepointSetGenerator; import java.io.Closeable;
import com.fasterxml.jackson.core.JsonParseException; import java.io.IOException;
import com.fasterxml.jackson.core.io.JsonStringEncoder; import java.lang.reflect.InvocationHandler;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.either;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.not;
public abstract class AbstractQueryTestCase<QB extends AbstractQueryBuilder<QB>> extends ESTestCase { public abstract class AbstractQueryTestCase<QB extends AbstractQueryBuilder<QB>> extends ESTestCase {
@ -418,13 +418,13 @@ public abstract class AbstractQueryTestCase<QB extends AbstractQueryBuilder<QB>>
} }
setSearchContext(randomTypes, context); setSearchContext(randomTypes, context);
Query secondLuceneQuery = rewriteQuery(secondQuery, context).toQuery(context); Query secondLuceneQuery = rewriteQuery(secondQuery, context).toQuery(context);
assertNotNull("toQuery should not return null", secondLuceneQuery);
assertLuceneQuery(secondQuery, secondLuceneQuery, context); assertLuceneQuery(secondQuery, secondLuceneQuery, context);
SearchContext.removeCurrent(); SearchContext.removeCurrent();
assertEquals("two equivalent query builders lead to different lucene queries", rewrite(secondLuceneQuery), rewrite(firstLuceneQuery)); assertEquals("two equivalent query builders lead to different lucene queries", rewrite(secondLuceneQuery), rewrite(firstLuceneQuery));
// if the initial lucene query is null, changing its boost won't have any effect, we shouldn't test that if (supportsBoostAndQueryName()) {
if (firstLuceneQuery != null && supportsBoostAndQueryName()) {
secondQuery.boost(firstQuery.boost() + 1f + randomFloat()); secondQuery.boost(firstQuery.boost() + 1f + randomFloat());
setSearchContext(randomTypes, context); setSearchContext(randomTypes, context);
Query thirdLuceneQuery = rewriteQuery(secondQuery, context).toQuery(context); Query thirdLuceneQuery = rewriteQuery(secondQuery, context).toQuery(context);