From c689e891d9ea53dca32bbb1e76501d8883c44043 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Wed, 8 Jul 2015 18:25:16 +0200 Subject: [PATCH] Query refactoring: SpanNearQueryBuilder and Parser Moving the query building functionality from the parser to the builders new toQuery() method analogous to other recent query refactorings. Relates to #10217 --- .../index/query/QueryBuilders.java | 4 +- .../index/query/SpanNearQueryBuilder.java | 152 +++++++++++++++--- .../index/query/SpanNearQueryParser.java | 36 ++--- .../query/SimpleIndexQueryParserTests.java | 2 +- .../index/query/SpanNearQueryBuilderTest.java | 85 ++++++++++ .../search/query/SearchQueryTests.java | 21 ++- .../migrate_query_refactoring.asciidoc | 5 + 7 files changed, 251 insertions(+), 54 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/index/query/SpanNearQueryBuilderTest.java diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java b/core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java index 5292b529d8885..179eb851baf3a 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java @@ -312,8 +312,8 @@ public static SpanFirstQueryBuilder spanFirstQuery(SpanQueryBuilder match, int e return new SpanFirstQueryBuilder(match, end); } - public static SpanNearQueryBuilder spanNearQuery() { - return new SpanNearQueryBuilder(); + public static SpanNearQueryBuilder spanNearQuery(int slop) { + return new SpanNearQueryBuilder(slop); } public static SpanNotQueryBuilder spanNotQuery() { diff --git a/core/src/main/java/org/elasticsearch/index/query/SpanNearQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/SpanNearQueryBuilder.java index 3fa6d21346f21..85e2f4b18616c 100644 --- a/core/src/main/java/org/elasticsearch/index/query/SpanNearQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/SpanNearQueryBuilder.java @@ -19,70 +19,180 @@ package org.elasticsearch.index.query; +import org.apache.lucene.search.Query; +import org.apache.lucene.search.spans.SpanNearQuery; +import org.apache.lucene.search.spans.SpanQuery; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.XContentBuilder; import java.io.IOException; import java.util.ArrayList; +import java.util.List; +import java.util.Objects; +/** + * Matches spans which are near one another. One can specify slop, the maximum number + * of intervening unmatched positions, as well as whether matches are required to be in-order. + * The span near query maps to Lucene {@link SpanNearQuery}. + */ public class SpanNearQueryBuilder extends AbstractQueryBuilder implements SpanQueryBuilder { public static final String NAME = "span_near"; - private ArrayList clauses = new ArrayList<>(); + /** Default for flag controlling whether matches are required to be in-order */ + public static boolean DEFAULT_IN_ORDER = true; + + /** Default for flag controlling whether payloads are collected */ + public static boolean DEFAULT_COLLECT_PAYLOADS = true; + + private final ArrayList clauses = new ArrayList<>(); - private Integer slop = null; + private final int slop; - private Boolean inOrder; + private boolean inOrder = DEFAULT_IN_ORDER; - private Boolean collectPayloads; + private boolean collectPayloads = DEFAULT_COLLECT_PAYLOADS; static final SpanNearQueryBuilder PROTOTYPE = new SpanNearQueryBuilder(); + /** + * @param slop controls the maximum number of intervening unmatched positions permitted + */ + public SpanNearQueryBuilder(int slop) { + this.slop = slop; + } + + /** + * only used for prototype + */ + private SpanNearQueryBuilder() { + this.slop = 0; + } + + /** + * @return the maximum number of intervening unmatched positions permitted + */ + public int slop() { + return this.slop; + } + public SpanNearQueryBuilder clause(SpanQueryBuilder clause) { - clauses.add(clause); + clauses.add(Objects.requireNonNull(clause)); return this; } - public SpanNearQueryBuilder slop(int slop) { - this.slop = slop; - return this; + /** + * @return the {@link SpanQueryBuilder} clauses that were set for this query + */ + public List clauses() { + return this.clauses; } + /** + * When inOrder is true, the spans from each clause + * must be in the same order as in clauses and must be non-overlapping. + * Defaults to true + */ public SpanNearQueryBuilder inOrder(boolean inOrder) { this.inOrder = inOrder; return this; } + /** + * @see SpanNearQueryBuilder#inOrder(boolean)) + */ + public boolean inOrder() { + return this.inOrder; + } + + /** + * @param collectPayloads flag controlling whether payloads are collected + */ public SpanNearQueryBuilder collectPayloads(boolean collectPayloads) { this.collectPayloads = collectPayloads; return this; } + /** + * @see SpanNearQueryBuilder#collectPayloads(boolean)) + */ + public boolean collectPayloads() { + return this.collectPayloads; + } + @Override protected void doXContent(XContentBuilder builder, Params params) throws IOException { - if (clauses.isEmpty()) { - throw new IllegalArgumentException("Must have at least one clause when building a spanNear query"); - } - if (slop == null) { - throw new IllegalArgumentException("Must set the slop when building a spanNear query"); - } builder.startObject(NAME); builder.startArray("clauses"); for (SpanQueryBuilder clause : clauses) { clause.toXContent(builder, params); } builder.endArray(); - builder.field("slop", slop.intValue()); - if (inOrder != null) { - builder.field("in_order", inOrder); - } - if (collectPayloads != null) { - builder.field("collect_payloads", collectPayloads); - } + builder.field("slop", slop); + builder.field("in_order", inOrder); + builder.field("collect_payloads", collectPayloads); printBoostAndQueryName(builder); builder.endObject(); } + @Override + protected Query doToQuery(QueryParseContext parseContext) throws IOException { + SpanQuery[] spanQueries = new SpanQuery[clauses.size()]; + for (int i = 0; i < clauses.size(); i++) { + Query query = clauses.get(i).toQuery(parseContext); + assert query instanceof SpanQuery; + spanQueries[i] = (SpanQuery) query; + } + return new SpanNearQuery(spanQueries, slop, inOrder, collectPayloads); + } + + @Override + public QueryValidationException validate() { + QueryValidationException validationExceptions = null; + if (clauses.isEmpty()) { + validationExceptions = addValidationError("query must include [clauses]", validationExceptions); + } + for (SpanQueryBuilder innerClause : clauses) { + validationExceptions = validateInnerQuery(innerClause, validationExceptions); + } + return validationExceptions; + } + + @Override + protected SpanNearQueryBuilder doReadFrom(StreamInput in) throws IOException { + SpanNearQueryBuilder queryBuilder = new SpanNearQueryBuilder(in.readVInt()); + List clauses = in.readNamedWriteableList(); + for (SpanQueryBuilder subClause : clauses) { + queryBuilder.clause(subClause); + } + queryBuilder.collectPayloads = in.readBoolean(); + queryBuilder.inOrder = in.readBoolean(); + return queryBuilder; + + } + + @Override + protected void doWriteTo(StreamOutput out) throws IOException { + out.writeVInt(slop); + out.writeNamedWriteableList(clauses); + out.writeBoolean(collectPayloads); + out.writeBoolean(inOrder); + } + + @Override + protected int doHashCode() { + return Objects.hash(clauses, slop, collectPayloads, inOrder); + } + + @Override + protected boolean doEquals(SpanNearQueryBuilder other) { + return Objects.equals(clauses, other.clauses) && + Objects.equals(slop, other.slop) && + Objects.equals(collectPayloads, other.collectPayloads) && + Objects.equals(inOrder, other.inOrder); + } + @Override public String getName() { return NAME; diff --git a/core/src/main/java/org/elasticsearch/index/query/SpanNearQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/SpanNearQueryParser.java index 41deb0d3190ee..2297104cd9b5a 100644 --- a/core/src/main/java/org/elasticsearch/index/query/SpanNearQueryParser.java +++ b/core/src/main/java/org/elasticsearch/index/query/SpanNearQueryParser.java @@ -19,9 +19,6 @@ package org.elasticsearch.index.query; -import org.apache.lucene.search.Query; -import org.apache.lucene.search.spans.SpanNearQuery; -import org.apache.lucene.search.spans.SpanQuery; import org.elasticsearch.common.Strings; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.xcontent.XContentParser; @@ -34,7 +31,7 @@ /** * */ -public class SpanNearQueryParser extends BaseQueryParserTemp { +public class SpanNearQueryParser extends BaseQueryParser { @Inject public SpanNearQueryParser() { @@ -46,16 +43,16 @@ public String[] names() { } @Override - public Query parse(QueryParseContext parseContext) throws IOException, QueryParsingException { + public QueryBuilder fromXContent(QueryParseContext parseContext) throws IOException, QueryParsingException { XContentParser parser = parseContext.parser(); float boost = AbstractQueryBuilder.DEFAULT_BOOST; Integer slop = null; - boolean inOrder = true; - boolean collectPayloads = true; + boolean inOrder = SpanNearQueryBuilder.DEFAULT_IN_ORDER; + boolean collectPayloads = SpanNearQueryBuilder.DEFAULT_COLLECT_PAYLOADS; String queryName = null; - List clauses = newArrayList(); + List clauses = newArrayList(); String currentFieldName = null; XContentParser.Token token; @@ -65,11 +62,11 @@ public Query parse(QueryParseContext parseContext) throws IOException, QueryPars } else if (token == XContentParser.Token.START_ARRAY) { if ("clauses".equals(currentFieldName)) { while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { - Query query = parseContext.parseInnerQuery(); - if (!(query instanceof SpanQuery)) { + QueryBuilder query = parseContext.parseInnerQueryBuilder(); + if (!(query instanceof SpanQueryBuilder)) { throw new QueryParsingException(parseContext, "spanNear [clauses] must be of type span query"); } - clauses.add((SpanQuery) query); + clauses.add((SpanQueryBuilder) query); } } else { throw new QueryParsingException(parseContext, "[span_near] query does not support [" + currentFieldName + "]"); @@ -92,19 +89,20 @@ public Query parse(QueryParseContext parseContext) throws IOException, QueryPars throw new QueryParsingException(parseContext, "[span_near] query does not support [" + currentFieldName + "]"); } } - if (clauses.isEmpty()) { - throw new QueryParsingException(parseContext, "span_near must include [clauses]"); - } + if (slop == null) { throw new QueryParsingException(parseContext, "span_near must include [slop]"); } - SpanNearQuery query = new SpanNearQuery(clauses.toArray(new SpanQuery[clauses.size()]), slop.intValue(), inOrder, collectPayloads); - query.setBoost(boost); - if (queryName != null) { - parseContext.addNamedQuery(queryName, query); + SpanNearQueryBuilder queryBuilder = new SpanNearQueryBuilder(slop); + for (SpanQueryBuilder subQuery : clauses) { + queryBuilder.clause(subQuery); } - return query; + queryBuilder.inOrder(inOrder); + queryBuilder.collectPayloads(collectPayloads); + queryBuilder.boost(boost); + queryBuilder.queryName(queryName); + return queryBuilder; } @Override diff --git a/core/src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java b/core/src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java index d0bdccebc256c..949d0ba143abc 100644 --- a/core/src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java @@ -1384,7 +1384,7 @@ public void testSpanFirstQuery() throws IOException { @Test public void testSpanNearQueryBuilder() throws IOException { IndexQueryParserService queryParser = queryParser(); - Query parsedQuery = queryParser.parse(spanNearQuery().clause(spanTermQuery("age", 34)).clause(spanTermQuery("age", 35)).clause(spanTermQuery("age", 36)).slop(12).inOrder(false).collectPayloads(false)).query(); + Query parsedQuery = queryParser.parse(spanNearQuery(12).clause(spanTermQuery("age", 34)).clause(spanTermQuery("age", 35)).clause(spanTermQuery("age", 36)).inOrder(false).collectPayloads(false)).query(); assertThat(parsedQuery, instanceOf(SpanNearQuery.class)); SpanNearQuery spanNearQuery = (SpanNearQuery) parsedQuery; assertThat(spanNearQuery.getClauses().length, equalTo(3)); diff --git a/core/src/test/java/org/elasticsearch/index/query/SpanNearQueryBuilderTest.java b/core/src/test/java/org/elasticsearch/index/query/SpanNearQueryBuilderTest.java new file mode 100644 index 0000000000000..8ac5adff80490 --- /dev/null +++ b/core/src/test/java/org/elasticsearch/index/query/SpanNearQueryBuilderTest.java @@ -0,0 +1,85 @@ +/* + * 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.index.query; + +import org.apache.lucene.search.Query; +import org.apache.lucene.search.spans.SpanNearQuery; +import org.apache.lucene.search.spans.SpanQuery; +import org.junit.Test; + +import java.io.IOException; +import java.util.List; + +public class SpanNearQueryBuilderTest extends BaseQueryTestCase { + + @Override + protected Query doCreateExpectedQuery(SpanNearQueryBuilder testQueryBuilder, QueryParseContext context) throws IOException { + List clauses = testQueryBuilder.clauses(); + SpanQuery[] spanQueries = new SpanQuery[clauses.size()]; + for (int i = 0; i < clauses.size(); i++) { + Query query = clauses.get(i).toQuery(context); + assert query instanceof SpanQuery; + spanQueries[i] = (SpanQuery) query; + } + return new SpanNearQuery(spanQueries, testQueryBuilder.slop(), testQueryBuilder.inOrder(), testQueryBuilder.collectPayloads()); + + } + + @Override + protected SpanNearQueryBuilder doCreateTestQueryBuilder() { + SpanNearQueryBuilder queryBuilder = new SpanNearQueryBuilder(randomIntBetween(-10, 10)); + int clauses = randomIntBetween(1, 6); + // we use one random SpanTermQueryBuilder to determine same field name for subsequent clauses + String fieldName = new SpanTermQueryBuilderTest().createTestQueryBuilder().fieldName(); + for (int i = 0; i < clauses; i++) { + // we need same field name in all clauses, so we only randomize value + Object value; + switch (fieldName) { + case BOOLEAN_FIELD_NAME: value = randomBoolean(); break; + case INT_FIELD_NAME: value = randomInt(); break; + case DOUBLE_FIELD_NAME: value = randomDouble(); break; + case STRING_FIELD_NAME: value = randomAsciiOfLengthBetween(1, 10); break; + default : value = randomAsciiOfLengthBetween(1, 10); + } + queryBuilder.clause(new SpanTermQueryBuilder(fieldName, value)); + } + queryBuilder.inOrder(randomBoolean()); + queryBuilder.collectPayloads(randomBoolean()); + return queryBuilder; + } + + @Test + public void testValidate() { + SpanNearQueryBuilder queryBuilder = new SpanNearQueryBuilder(1); + assertValidate(queryBuilder, 1); // empty clause list + + int totalExpectedErrors = 0; + int clauses = randomIntBetween(1, 10); + for (int i = 0; i < clauses; i++) { + if (randomBoolean()) { + queryBuilder.clause(new SpanTermQueryBuilder("", "test")); + totalExpectedErrors++; + } else { + queryBuilder.clause(new SpanTermQueryBuilder("name", "value")); + } + } + assertValidate(queryBuilder, totalExpectedErrors); + } +} diff --git a/core/src/test/java/org/elasticsearch/search/query/SearchQueryTests.java b/core/src/test/java/org/elasticsearch/search/query/SearchQueryTests.java index 9ddddea409870..e5c7dcdd8afec 100644 --- a/core/src/test/java/org/elasticsearch/search/query/SearchQueryTests.java +++ b/core/src/test/java/org/elasticsearch/search/query/SearchQueryTests.java @@ -1578,10 +1578,9 @@ public void testSimpleSpan() throws IOException, ExecutionException, Interrupted assertHitCount(searchResponse, 1l); searchResponse = client().prepareSearch("test").setQuery( - spanNearQuery() + spanNearQuery(3) .clause(spanTermQuery("description", "foo")) - .clause(spanTermQuery("description", "other")) - .slop(3)).get(); + .clause(spanTermQuery("description", "other"))).get(); assertHitCount(searchResponse, 3l); } @@ -1628,28 +1627,28 @@ public void testSpanNot() throws IOException, ExecutionException, InterruptedExc refresh(); SearchResponse searchResponse = client().prepareSearch("test") - .setQuery(spanNotQuery().include(spanNearQuery() + .setQuery(spanNotQuery().include(spanNearQuery(1) .clause(QueryBuilders.spanTermQuery("description", "quick")) - .clause(QueryBuilders.spanTermQuery("description", "fox")).slop(1)).exclude(spanTermQuery("description", "brown"))).get(); + .clause(QueryBuilders.spanTermQuery("description", "fox"))).exclude(spanTermQuery("description", "brown"))).get(); assertHitCount(searchResponse, 1l); searchResponse = client().prepareSearch("test") - .setQuery(spanNotQuery().include(spanNearQuery() + .setQuery(spanNotQuery().include(spanNearQuery(1) .clause(QueryBuilders.spanTermQuery("description", "quick")) - .clause(QueryBuilders.spanTermQuery("description", "fox")).slop(1)).exclude(spanTermQuery("description", "sleeping")).dist(5)).get(); + .clause(QueryBuilders.spanTermQuery("description", "fox"))).exclude(spanTermQuery("description", "sleeping")).dist(5)).get(); assertHitCount(searchResponse, 1l); searchResponse = client().prepareSearch("test") - .setQuery(spanNotQuery().include(spanNearQuery() + .setQuery(spanNotQuery().include(spanNearQuery(1) .clause(QueryBuilders.spanTermQuery("description", "quick")) - .clause(QueryBuilders.spanTermQuery("description", "fox")).slop(1)).exclude(spanTermQuery("description", "jumped")).pre(1).post(1)).get(); + .clause(QueryBuilders.spanTermQuery("description", "fox"))).exclude(spanTermQuery("description", "jumped")).pre(1).post(1)).get(); assertHitCount(searchResponse, 1l); try { client().prepareSearch("test") - .setQuery(spanNotQuery().include(spanNearQuery() + .setQuery(spanNotQuery().include(spanNearQuery(1) .clause(QueryBuilders.spanTermQuery("description", "quick")) - .clause(QueryBuilders.spanTermQuery("description", "fox")).slop(1)).exclude(spanTermQuery("description", "jumped")).dist(2).pre(2) + .clause(QueryBuilders.spanTermQuery("description", "fox"))).exclude(spanTermQuery("description", "jumped")).dist(2).pre(2) ).get(); fail("ElasticsearchIllegalArgumentException should have been caught"); } catch (ElasticsearchException e) { diff --git a/docs/reference/migration/migrate_query_refactoring.asciidoc b/docs/reference/migration/migrate_query_refactoring.asciidoc index 7962d70d6b1a1..9e8e472d4dda4 100644 --- a/docs/reference/migration/migrate_query_refactoring.asciidoc +++ b/docs/reference/migration/migrate_query_refactoring.asciidoc @@ -17,6 +17,11 @@ Removed setters for mandatory big/little inner span queries. Both arguments now to be supplied at construction time already and have to be non-null. Updated static factory methods in QueryBuilders accordingly. +==== SpanNearQueryBuilder + +Removed setter for mandatory slop parameter, needs to be set in constructor now. +Updated the static factory methods in QueryBuilders accordingly. + ==== QueryFilterBuilder Removed the setter `queryName(String queryName)` since this field is not supported