Skip to content

Query refactoring: SpanNotQueryBuilder and Parser #12365

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 22, 2015
Merged

Query refactoring: SpanNotQueryBuilder and Parser #12365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,8 +316,8 @@ public static SpanNearQueryBuilder spanNearQuery(int slop) {
return new SpanNearQueryBuilder(slop);
}

public static SpanNotQueryBuilder spanNotQuery() {
return new SpanNotQueryBuilder();
public static SpanNotQueryBuilder spanNotQuery(SpanQueryBuilder include, SpanQueryBuilder exclude) {
return new SpanNotQueryBuilder(include, exclude);
}

public static SpanOrQueryBuilder spanOrQuery() {
Expand Down
156 changes: 123 additions & 33 deletions core/src/main/java/org/elasticsearch/index/query/SpanNotQueryBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,82 +19,172 @@

package org.elasticsearch.index.query;

import org.apache.lucene.search.Query;
import org.apache.lucene.search.spans.SpanNotQuery;
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.Objects;

public class SpanNotQueryBuilder extends AbstractQueryBuilder<SpanNotQueryBuilder> implements SpanQueryBuilder<SpanNotQueryBuilder> {

public static final String NAME = "span_not";

private SpanQueryBuilder include;
/** the default pre parameter size */
public static final int DEFAULT_PRE = 0;
/** the default post parameter size */
public static final int DEFAULT_POST = 0;

private SpanQueryBuilder exclude;
private final SpanQueryBuilder include;

private Integer dist;
private final SpanQueryBuilder exclude;

private Integer pre;
private int pre = DEFAULT_PRE;

private Integer post;
private int post = DEFAULT_POST;

static final SpanNotQueryBuilder PROTOTYPE = new SpanNotQueryBuilder();

public SpanNotQueryBuilder include(SpanQueryBuilder include) {
this.include = include;
return this;
/**
* Construct a span query matching spans from <code>include</code> which
* have no overlap with spans from <code>exclude</code>.
* @param include the span query whose matches are filtered
* @param exclude the span query whose matches must not overlap
*/
public SpanNotQueryBuilder(SpanQueryBuilder include, SpanQueryBuilder exclude) {
this.include = Objects.requireNonNull(include);
this.exclude = Objects.requireNonNull(exclude);
}

public SpanNotQueryBuilder exclude(SpanQueryBuilder exclude) {
this.exclude = exclude;
return this;
// only used for prototype
private SpanNotQueryBuilder() {
this.include = null;
this.exclude = null;
}

/**
* @return the span query whose matches are filtered
*/
public SpanQueryBuilder include() {
return this.include;
}

/**
* @return the span query whose matches must not overlap
*/
public SpanQueryBuilder exclude() {
return this.exclude;
}

/**
* @param dist the amount of tokens from within the include span can’t have overlap with the exclude span.
* Equivalent to setting both pre and post parameter.
*/
public SpanNotQueryBuilder dist(int dist) {
this.dist = dist;
pre(dist);
post(dist);
return this;
}

/**
* @param pre the amount of tokens before the include span that can’t have overlap with the exclude span. Values
Copy link
Member

@javanna javanna Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we lost dist here?

Copy link
Member Author

@cbuescher cbuescher Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting dist is equivalent to setting pre/post to the same value, so I removed the internal representation for dist entirely. Only caveat here is that if user sets dist on the builder, query is logged with per/post fields, but since we said doXContent will be used for logging / debugging only after refactoring this is similar to outputting defaults that user hasn't explicitely set, I think.

Copy link
Member

@javanna javanna Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright, sounds good

Copy link
Member

@javanna javanna Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should start thinking about testing the parsing phase for things that we never output from doXContent.... :)

Copy link
Member Author

@cbuescher cbuescher Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can start doing this here, only problem is creating the JSON without the builder is kind of messy. But I will add a test for dist parameter for the parser.

Copy link
Member

@javanna javanna Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know what you mean about creating the json, plain strings would work I guess... we have this same problem in other queries (e.g. term query)...something we have to keep in mind for later.

* smaller than 0 will be ignored and 0 used instead.
*/
public SpanNotQueryBuilder pre(int pre) {
this.pre = (pre >=0) ? pre : 0;
this.pre = (pre >= 0) ? pre : 0;
return this;
}

/**
* @return the amount of tokens before the include span that can’t have overlap with the exclude span.
* @see SpanNotQueryBuilder#pre(int)
*/
public Integer pre() {
return this.pre;
}

/**
* @param post the amount of tokens after the include span that can’t have overlap with the exclude span.
*/
public SpanNotQueryBuilder post(int post) {
this.post = (post >= 0) ? post : 0;
return this;
}

/**
* @return the amount of tokens after the include span that can’t have overlap with the exclude span.
* @see SpanNotQueryBuilder#post(int)
*/
public Integer post() {
return this.post;
}

@Override
protected void doXContent(XContentBuilder builder, Params params) throws IOException {
if (include == null) {
throw new IllegalArgumentException("Must specify include when using spanNot query");
}
if (exclude == null) {
throw new IllegalArgumentException("Must specify exclude when using spanNot query");
}

if (dist != null && (pre != null || post != null)) {
throw new IllegalArgumentException("spanNot can either use [dist] or [pre] & [post] (or none)");
}

builder.startObject(NAME);
builder.field("include");
include.toXContent(builder, params);
builder.field("exclude");
exclude.toXContent(builder, params);
if (dist != null) {
builder.field("dist", dist);
}
if (pre != null) {
builder.field("pre", pre);
}
if (post != null) {
builder.field("post", post);
}
builder.field("pre", pre);
builder.field("post", post);
printBoostAndQueryName(builder);
builder.endObject();
}

@Override
protected Query doToQuery(QueryParseContext parseContext) throws IOException {

Query includeQuery = this.include.toQuery(parseContext);
assert includeQuery instanceof SpanQuery;
Query excludeQuery = this.exclude.toQuery(parseContext);
assert excludeQuery instanceof SpanQuery;
Copy link
Member

@javanna javanna Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have problems with LateParsingQuery here too? maybe an if instead of an assert?

Copy link
Member Author

@cbuescher cbuescher Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, already discussed in #12342 (comment)


SpanNotQuery query = new SpanNotQuery((SpanQuery) includeQuery, (SpanQuery) excludeQuery, pre, post);
return query;
}

@Override
public QueryValidationException validate() {
QueryValidationException validationExceptions = validateInnerQuery(include, null);
validationExceptions = validateInnerQuery(exclude, validationExceptions);
return validationExceptions;
}

@Override
protected SpanNotQueryBuilder doReadFrom(StreamInput in) throws IOException {
SpanQueryBuilder include = in.readNamedWriteable();
SpanQueryBuilder exclude = in.readNamedWriteable();
SpanNotQueryBuilder queryBuilder = new SpanNotQueryBuilder(include, exclude);
queryBuilder.pre(in.readVInt());
queryBuilder.post(in.readVInt());
return queryBuilder;
}

@Override
protected void doWriteTo(StreamOutput out) throws IOException {
out.writeNamedWriteable(include);
out.writeNamedWriteable(exclude);
out.writeVInt(pre);
out.writeVInt(post);
}

@Override
protected int doHashCode() {
return Objects.hash(include, exclude, pre, post);
}

@Override
protected boolean doEquals(SpanNotQueryBuilder other) {
return Objects.equals(include, other.include) &&
Objects.equals(exclude, other.exclude) &&
(pre == other.pre) &&
(post == other.post);
}

@Override
public String getName() {
return NAME;
Expand Down
51 changes: 20 additions & 31 deletions core/src/main/java/org/elasticsearch/index/query/SpanNotQueryParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,6 @@

package org.elasticsearch.index.query;

import org.apache.lucene.search.Query;
import org.apache.lucene.search.spans.SpanNotQuery;
import org.apache.lucene.search.spans.SpanQuery;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.xcontent.XContentParser;
Expand All @@ -31,7 +28,7 @@
/**
*
*/
public class SpanNotQueryParser extends BaseQueryParserTemp {
public class SpanNotQueryParser extends BaseQueryParser {

@Inject
public SpanNotQueryParser() {
Expand All @@ -43,13 +40,13 @@ 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;

SpanQuery include = null;
SpanQuery exclude = null;
SpanQueryBuilder include = null;
SpanQueryBuilder exclude = null;

Integer dist = null;
Integer pre = null;
Expand All @@ -64,17 +61,17 @@ public Query parse(QueryParseContext parseContext) throws IOException, QueryPars
currentFieldName = parser.currentName();
} else if (token == XContentParser.Token.START_OBJECT) {
if ("include".equals(currentFieldName)) {
Query query = parseContext.parseInnerQuery();
if (!(query instanceof SpanQuery)) {
QueryBuilder query = parseContext.parseInnerQueryBuilder();
if (!(query instanceof SpanQueryBuilder)) {
throw new QueryParsingException(parseContext, "spanNot [include] must be of type span query");
}
include = (SpanQuery) query;
include = (SpanQueryBuilder) query;
} else if ("exclude".equals(currentFieldName)) {
Query query = parseContext.parseInnerQuery();
if (!(query instanceof SpanQuery)) {
QueryBuilder query = parseContext.parseInnerQueryBuilder();
if (!(query instanceof SpanQueryBuilder)) {
throw new QueryParsingException(parseContext, "spanNot [exclude] must be of type span query");
}
exclude = (SpanQuery) query;
exclude = (SpanQueryBuilder) query;
} else {
throw new QueryParsingException(parseContext, "[span_not] query does not support [" + currentFieldName + "]");
}
Expand Down Expand Up @@ -104,27 +101,19 @@ public Query parse(QueryParseContext parseContext) throws IOException, QueryPars
throw new QueryParsingException(parseContext, "spanNot can either use [dist] or [pre] & [post] (or none)");
}

// set appropriate defaults
if (pre != null && post == null) {
post = 0;
} else if (pre == null && post != null){
pre = 0;
SpanNotQueryBuilder spanNotQuery = new SpanNotQueryBuilder(include, exclude);
if (dist != null) {
spanNotQuery.dist(dist);
}

SpanNotQuery query;
if (pre != null && post != null) {
query = new SpanNotQuery(include, exclude, pre, post);
} else if (dist != null) {
query = new SpanNotQuery(include, exclude, dist);
} else {
query = new SpanNotQuery(include, exclude);
if (pre != null) {
spanNotQuery.pre(pre);
}

query.setBoost(boost);
if (queryName != null) {
parseContext.addNamedQuery(queryName, query);
if (post != null) {
spanNotQuery.post(post);
}
return query;
spanNotQuery.boost(boost);
spanNotQuery.queryName(queryName);
return spanNotQuery;
}

@Override
Expand Down
3 changes: 1 addition & 2 deletions core/src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
import org.apache.lucene.queries.TermsQuery;
import org.apache.lucene.search.*;
import org.apache.lucene.search.BooleanClause.Occur;
import org.apache.lucene.search.MultiTermQuery.RewriteMethod;
import org.apache.lucene.search.join.ToParentBlockJoinQuery;
import org.apache.lucene.search.spans.*;
import org.apache.lucene.spatial.prefix.IntersectsPrefixTreeFilter;
Expand Down Expand Up @@ -1323,7 +1322,7 @@ public void testSpanTermQuery() throws IOException {
@Test
public void testSpanNotQueryBuilder() throws IOException {
IndexQueryParserService queryParser = queryParser();
Query parsedQuery = queryParser.parse(spanNotQuery().include(spanTermQuery("age", 34)).exclude(spanTermQuery("age", 35))).query();
Query parsedQuery = queryParser.parse(spanNotQuery(spanTermQuery("age", 34), spanTermQuery("age", 35))).query();
assertThat(parsedQuery, instanceOf(SpanNotQuery.class));
SpanNotQuery spanNotQuery = (SpanNotQuery) parsedQuery;
// since age is automatically registered in data, we encode it as numeric
Expand Down
Loading