Query refactoring: SpanWithinQueryBuilder 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

core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java

@@ -324,9 +324,12 @@ public abstract class QueryBuilders {
         return new SpanOrQueryBuilder();
     }
 
-    /** Creates a new {@code span_within} builder. */
+    /** Creates a new {@code span_within} builder.
-    public static SpanWithinQueryBuilder spanWithinQuery() {
+    * @param big the big clause, it must enclose {@code little} for a match.
-        return new SpanWithinQueryBuilder();
+    * @param little the little clause, it must be contained within {@code big} for a match.
+    */
+    public static SpanWithinQueryBuilder spanWithinQuery(SpanQueryBuilder big, SpanQueryBuilder little) {
+        return new SpanWithinQueryBuilder(big, little);
     }
 
     /**

core/src/main/java/org/elasticsearch/index/query/SpanWithinQueryBuilder.java

@@ -19,9 +19,15 @@
 
 package org.elasticsearch.index.query;
 
+import org.apache.lucene.search.Query;
+import org.apache.lucene.search.spans.SpanQuery;
+import org.apache.lucene.search.spans.SpanWithinQuery;
+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.Objects;
 
 /**
  * Builder for {@link org.apache.lucene.search.spans.SpanWithinQuery}.
@@ -29,34 +35,44 @@ import java.io.IOException;
 public class SpanWithinQueryBuilder extends AbstractQueryBuilder<SpanWithinQueryBuilder> implements SpanQueryBuilder<SpanWithinQueryBuilder> {
 
     public static final String NAME = "span_within";
-    private SpanQueryBuilder big;
+    private final SpanQueryBuilder big;
-    private SpanQueryBuilder little;
+    private final SpanQueryBuilder little;
     static final SpanWithinQueryBuilder PROTOTYPE = new SpanWithinQueryBuilder();
 
     /**
-     * Sets the little clause, it must be contained within {@code big} for a match.
+     * Query that returns spans from <code>little</code> that are contained in a spans from <code>big</code>.
+     * @param big clause that must enclose {@code little} for a match.
+     * @param little the little clause, it must be contained within {@code big} for a match.
      */
-    public SpanWithinQueryBuilder little(SpanQueryBuilder clause) {
+    public SpanWithinQueryBuilder(SpanQueryBuilder big, SpanQueryBuilder little) {
-        this.little = clause;
+        this.little = Objects.requireNonNull(little);
-        return this;
+        this.big = Objects.requireNonNull(big);
     }
 
     /**
-     * Sets the big clause, it must enclose {@code little} for a match.
+     * for prototype only
      */
-    public SpanWithinQueryBuilder big(SpanQueryBuilder clause) {
+    private SpanWithinQueryBuilder() {
-        this.big = clause;
+        this.little = null;
-        return this;
+        this.big = null;
+    }
+
+    /**
+     * @return the little clause, contained within {@code big} for a match.
+     */
+    public SpanQueryBuilder little() {
+        return this.little;
+    }
+
+    /**
+     * @return the big clause that must enclose {@code little} for a match.
+     */
+    public SpanQueryBuilder big() {
+        return this.big;
     }
 
     @Override
     protected void doXContent(XContentBuilder builder, Params params) throws IOException {
-        if (big == null) {
-            throw new IllegalArgumentException("Must specify big clause when building a span_within query");
-        }
-        if (little == null) {
-            throw new IllegalArgumentException("Must specify little clause when building a span_within query");
-        }
         builder.startObject(NAME);
 
         builder.field("big");
@@ -70,6 +86,46 @@ public class SpanWithinQueryBuilder extends AbstractQueryBuilder<SpanWithinQuery
         builder.endObject();
     }
 
+    @Override
+    protected Query doToQuery(QueryParseContext parseContext) throws IOException {
+        Query innerBig = big.toQuery(parseContext);
+        assert innerBig instanceof SpanQuery;
+        Query innerLittle = little.toQuery(parseContext);
+        assert innerLittle instanceof SpanQuery;
+        return new SpanWithinQuery((SpanQuery) innerBig, (SpanQuery) innerLittle);
+    }
+
+    @Override
+    public QueryValidationException validate() {
+        QueryValidationException validationExceptions = validateInnerQuery(big, null);
+        validationExceptions = validateInnerQuery(little, validationExceptions);
+        return validationExceptions;
+    }
+
+    @Override
+    protected SpanWithinQueryBuilder doReadFrom(StreamInput in) throws IOException {
+        SpanQueryBuilder big = in.readNamedWriteable();
+        SpanQueryBuilder little = in.readNamedWriteable();
+        return new SpanWithinQueryBuilder(big, little);
+    }
+
+    @Override
+    protected void doWriteTo(StreamOutput out) throws IOException {
+        out.writeNamedWriteable(big);
+        out.writeNamedWriteable(little);
+    }
+
+    @Override
+    protected int doHashCode() {
+        return Objects.hash(big, little);
+    }
+
+    @Override
+    protected boolean doEquals(SpanWithinQueryBuilder other) {
+        return Objects.equals(big, other.big) &&
+               Objects.equals(little, other.little);
+    }
+
     @Override
     public String getName() {
         return NAME;

core/src/main/java/org/elasticsearch/index/query/SpanWithinQueryParser.java

@@ -19,8 +19,6 @@
 
 package org.elasticsearch.index.query;
 
-import org.apache.lucene.search.Query;
-import org.apache.lucene.search.spans.SpanQuery;
 import org.apache.lucene.search.spans.SpanWithinQuery;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.inject.Inject;
@@ -31,7 +29,7 @@ import java.io.IOException;
 /**
  * Parser for {@link SpanWithinQuery}
  */
-public class SpanWithinQueryParser extends BaseQueryParserTemp {
+public class SpanWithinQueryParser extends BaseQueryParser {
 
     @Inject
     public SpanWithinQueryParser() {
@@ -43,13 +41,13 @@ public class SpanWithinQueryParser extends BaseQueryParserTemp {
     }
 
     @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;
         String queryName = null;
-        SpanQuery big = null;
+        SpanQueryBuilder big = null;
-        SpanQuery little = null;
+        SpanQueryBuilder little = null;
 
         String currentFieldName = null;
         XContentParser.Token token;
@@ -58,17 +56,17 @@ public class SpanWithinQueryParser extends BaseQueryParserTemp {
                 currentFieldName = parser.currentName();
             } else if (token == XContentParser.Token.START_OBJECT) {
                 if ("big".equals(currentFieldName)) {
-                    Query query = parseContext.parseInnerQuery();
+                    QueryBuilder query = parseContext.parseInnerQueryBuilder();
-                    if (query instanceof SpanQuery == false) {
+                    if (query instanceof SpanQueryBuilder == false) {
                         throw new QueryParsingException(parseContext, "span_within [big] must be of type span query");
                     }
-                    big = (SpanQuery) query;
+                    big = (SpanQueryBuilder) query;
                 } else if ("little".equals(currentFieldName)) {
-                    Query query = parseContext.parseInnerQuery();
+                    QueryBuilder query = parseContext.parseInnerQueryBuilder();
-                    if (query instanceof SpanQuery == false) {
+                    if (query instanceof SpanQueryBuilder == false) {
                         throw new QueryParsingException(parseContext, "span_within [little] must be of type span query");
                     }
-                    little = (SpanQuery) query;
+                    little = (SpanQueryBuilder) query;
                 } else {
                     throw new QueryParsingException(parseContext, "[span_within] query does not support [" + currentFieldName + "]");
                 }
@@ -88,11 +86,8 @@ public class SpanWithinQueryParser extends BaseQueryParserTemp {
             throw new QueryParsingException(parseContext, "span_within must include [little]");
         }
 
-        Query query = new SpanWithinQuery(big, little);
+        SpanWithinQueryBuilder query = new SpanWithinQueryBuilder(big, little);
-        query.setBoost(boost);
+        query.boost(boost).queryName(queryName);
-        if (queryName != null) {
-            parseContext.addNamedQuery(queryName, query);
-        }
         return query;
     }
 

core/src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java

@@ -1347,9 +1347,7 @@ public class SimpleIndexQueryParserTests extends ElasticsearchSingleNodeTest {
         IndexQueryParserService queryParser = queryParser();
         Query expectedQuery = new SpanWithinQuery(new SpanTermQuery(new Term("age", longToPrefixCoded(34, 0))),
                                                   new SpanTermQuery(new Term("age", longToPrefixCoded(35, 0))));
-        Query actualQuery = queryParser.parse(spanWithinQuery()
+        Query actualQuery = queryParser.parse(spanWithinQuery(spanTermQuery("age", 34), spanTermQuery("age", 35)))
-                                              .big(spanTermQuery("age", 34))
-                                              .little(spanTermQuery("age", 35)))
                                               .query();
         assertEquals(expectedQuery, actualQuery);
     }

core/src/test/java/org/elasticsearch/index/query/SpanWithinQueryBuilderTest.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.SpanQuery;
+import org.apache.lucene.search.spans.SpanWithinQuery;
+import org.junit.Test;
+
+import java.io.IOException;
+
+public class SpanWithinQueryBuilderTest extends BaseQueryTestCase<SpanWithinQueryBuilder> {
+
+    @Override
+    protected Query doCreateExpectedQuery(SpanWithinQueryBuilder testQueryBuilder, QueryParseContext context) throws IOException {
+        SpanQuery big = (SpanQuery) testQueryBuilder.big().toQuery(context);
+        SpanQuery little = (SpanQuery) testQueryBuilder.little().toQuery(context);
+        return new SpanWithinQuery(big, little);
+    }
+
+    @Override
+    protected SpanWithinQueryBuilder doCreateTestQueryBuilder() {
+        SpanTermQueryBuilder bigQuery = new SpanTermQueryBuilderTest().createTestQueryBuilder();
+        // we need same field name and value type as bigQuery for little query
+        String fieldName = bigQuery.fieldName();
+        Object littleValue;
+        switch (fieldName) {
+            case BOOLEAN_FIELD_NAME: littleValue = randomBoolean(); break;
+            case INT_FIELD_NAME: littleValue = randomInt(); break;
+            case DOUBLE_FIELD_NAME: littleValue = randomDouble(); break;
+            case STRING_FIELD_NAME: littleValue = randomAsciiOfLengthBetween(1, 10); break;
+            default : littleValue = randomAsciiOfLengthBetween(1, 10);
+        }
+        SpanTermQueryBuilder littleQuery = new SpanTermQueryBuilder(fieldName, littleValue);
+        return new SpanWithinQueryBuilder(bigQuery, littleQuery);
+    }
+
+    @Test
+    public void testValidate() {
+        int totalExpectedErrors = 0;
+        SpanQueryBuilder bigSpanQueryBuilder;
+        if (randomBoolean()) {
+            bigSpanQueryBuilder = new SpanTermQueryBuilder("", "test");
+            totalExpectedErrors++;
+        } else {
+            bigSpanQueryBuilder = new SpanTermQueryBuilder("name", "value");
+        }
+        SpanQueryBuilder littleSpanQueryBuilder;
+        if (randomBoolean()) {
+            littleSpanQueryBuilder = new SpanTermQueryBuilder("", "test");
+            totalExpectedErrors++;
+        } else {
+            littleSpanQueryBuilder = new SpanTermQueryBuilder("name", "value");
+        }
+        SpanWithinQueryBuilder queryBuilder = new SpanWithinQueryBuilder(bigSpanQueryBuilder, littleSpanQueryBuilder);
+        assertValidate(queryBuilder, totalExpectedErrors);
+    }
+
+    @Test(expected=NullPointerException.class)
+    public void testNullBig() {
+        new SpanWithinQueryBuilder(null, new SpanTermQueryBuilder("name", "value"));
+    }
+
+    @Test(expected=NullPointerException.class)
+    public void testNullLittle() {
+        new SpanWithinQueryBuilder(new SpanTermQueryBuilder("name", "value"), null);
+    }
+}

docs/reference/migration/migrate_query_refactoring.asciidoc

@@ -27,6 +27,13 @@ Updated the static factory methods in QueryBuilders accordingly.
 Removed setter for mandatory include/exclude span query clause, needs to be set in constructor now.
 Updated the static factory methods in QueryBuilders and tests accordingly.
 
+==== SpanWithinQueryBuilder
+
+Removed setters for mandatory big/little inner span queries. Both arguments now have
+to be supplied at construction time already and have to be non-null. Updated
+static factory methods in QueryBuilders accordingly.
+>>>>>>> Query refactoring: SpanWithinQueryBuilder and Parser
+
 ==== QueryFilterBuilder
 
 Removed the setter `queryName(String queryName)` since this field is not supported