Skip to content
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.

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

Handle empty query bodies at parse time and remove EmptyQueryBuilder #17624

Merged
merged 4 commits into from Jun 2, 2016

Conversation

Projects
None yet
5 participants
@cbuescher
Copy link
Member

commented Apr 8, 2016

As stated in #17540 we currently support empty query bodies like the filter in

"constant_score" : {  "filter" : { } }

in the query DSL. How these empty query bodies are handled depends on the query they are contained in. Upstream they are usually either ignored, converted to match all or no documents or bubbled up further in the query tree. While parsing they are currently represented as EmptyQueryBuilders, which serve no other purpose as to signal callers that it is the result of parsing an empty query body.
When not handled anywhere else, it needs to be checked for on the shard when building the lucene query. This is trappy, so this PR changes the parsing of compound queries. Instead of returning QueryBuilder, the core query parsing method QueryShardContext#parseInnerQueryBuilder() now return an Optional which is empty in the case of queries with empty body. This has the advantage of forcing caller code to deal with this sooner or later. when encountering empty Optionals, compound query builders now have the choice to ignore them, pass them on or rewrite to a different query, depending on context.

Closes #17541

@cbuescher

View changes

core/src/main/java/org/elasticsearch/index/percolator/PercolatorFieldMapper.java Outdated
@@ -201,7 +197,8 @@ static Query toQuery(QueryShardContext context, boolean mapUnmappedFieldsAsStrin
static QueryBuilder<?> parseQueryBuilder(QueryParseContext context, XContentParser parser) {
context.reset(parser);
try {
return context.parseInnerQueryBuilder();
// NORELEASE not really sure what to do here
return context.parseInnerQueryBuilder().get();

This comment has been minimized.

Copy link
@cbuescher

cbuescher Apr 8, 2016

Author Member

I'm nor sure if this should error or ignore if the query hasn't been resolved to something at his point. Maybe @martijnvg has an opinion?

@@ -38,7 +39,7 @@
* call
* @return the new QueryBuilder
*/
QB fromXContent(QueryParseContext parseContext) throws IOException;
Optional<QB> fromXContent(QueryParseContext parseContext) throws IOException;

This comment has been minimized.

Copy link
@cbuescher

cbuescher Apr 8, 2016

Author Member

Needed to change this signature so some QueryBuilders (like Boosting, ConstantScore, HasChild...) have the change to deffer resolving an empty Optional to something (thats what they also used to do before by returning 'null' to their caller in parse() (on 2.0) and now in doToQuery().

@cbuescher cbuescher force-pushed the cbuescher:remove-empty-querybuilder branch 5 times, most recently Apr 12, 2016

@cbuescher cbuescher force-pushed the cbuescher:remove-empty-querybuilder branch 6 times, most recently Apr 20, 2016

@cbuescher cbuescher force-pushed the cbuescher:remove-empty-querybuilder branch 2 times, most recently Apr 27, 2016

@cbuescher cbuescher force-pushed the cbuescher:remove-empty-querybuilder branch 4 times, most recently May 6, 2016

@colings86

This comment has been minimized.

Copy link
Member

commented May 16, 2016

This looks good to me but I wonder if we should get another person to look at it (maybe @martijnvg @jpountz or @javanna ?) to get their opinion on the approach. The reason for this is that this is quite a big change to the query parsing (touches every query) to deal with a case that we are going to remove in 6.0 anyway. So although this is an improvement for 5.x to deal with the {} we are probably going to reverse this change in 6.0 and not return Optional from the query parse method since in 6.0 the parse method will always need to return a QueryBuilder and will not be allow to return null or anything that represents {}.

@jpountz

This comment has been minimized.

Copy link
Contributor

commented May 16, 2016

Agreed: I would rather like a soon-to-be-deprecated feature to be a bit buggy than to have deep implications on our internal APIs.

@cbuescher cbuescher force-pushed the cbuescher:remove-empty-querybuilder branch May 17, 2016

@cbuescher

This comment has been minimized.

Copy link
Member Author

commented May 17, 2016

@colings86 @jpountz thanks for taking a look. I would still like to argue for getting this in: we only introduced the EmptyQueryBuilder as a means to handle the empty clauses on master to avoid having to deal with null values during the query refactoring. This is not really a bug, but it is really just a crutch that we would have avoided if we had had Optional when we introduced it (@javanna might agree here). Now that we can represent optional values differently, I think we should do that rather than introduce a query builder that can get missused. Even if we plan to remove it in 6.0, it will stick around for a shile, but I think Optional is a much better choice with Java 8. I also think the changes to query builder method signatures are rather small in most cases (just wrapping the builder in an Optional), but we don't have to transport those objects across the wire anymore if we deal with empty clauses on the coordinating node already.

@javanna

This comment has been minimized.

Copy link
Member

commented Jun 1, 2016

I had another look and left a few questions but this looks very good!

cbuescher added some commits Apr 20, 2016

Handle empty query bodies at parse time and remove EmptyQueryBuilder
Currently we support empty query clauses like the filter in

"constant_score" : {  "filter" : { } }

How these clauses are handled depends on the surrounding query.
They later are either ignored, converted to match all or no documents or
passed up further in the query hierarchy. During parsing these claues are
currently represented as EmptyQueryBuilders. When not handled anywhere else,
these special cases need to be checked for on the shard when building the
lucene query.

This is trappy, so this PR changes the parsing of compound queries. Instead
of returning QueryBuilder, the core query parsing method
QueryShardContext#parseInnerQueryBuilder() now return an Optional which can
be empty in the case of empty query clauses. This has the advantage of forcing
callers to deal with this sooner or later. When encountering empty Optionals,
compound query builders now have the choice to ignore them, pass them on or
rewrite to a different query, depending on context.

@cbuescher cbuescher force-pushed the cbuescher:remove-empty-querybuilder branch 2 times, most recently Jun 2, 2016

@cbuescher

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2016

@javanna thanke, I hope I answered and adressed all of your review comments above and in the last commit.

@@ -399,6 +399,7 @@ public void testToQuery() throws IOException {
QB controlQuery = copyQuery(firstQuery);
setSearchContext(randomTypes, context); // only set search context for toQuery to be more realistic
Query firstLuceneQuery = rewriteQuery(firstQuery, context).toQuery(context);
assertNotNull("toQuery should not return null", firstLuceneQuery);

This comment has been minimized.

Copy link
@javanna

javanna Jun 2, 2016

Member

shall we do the same with the second lucene query and other possible queries we have here?

This comment has been minimized.

Copy link
@cbuescher

cbuescher Jun 2, 2016

Author Member

sure, done.

@javanna

View changes

core/src/main/java/org/elasticsearch/index/query/DisMaxQueryBuilder.java Outdated
@@ -114,11 +115,13 @@ public float tieBreaker() {
protected void doXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject(NAME);
builder.field(TIE_BREAKER_FIELD.getPreferredName(), tieBreaker);
if (queries.isEmpty() == false) {

This comment has been minimized.

Copy link
@javanna

javanna Jun 2, 2016

Member

indentation looks off here. btw shall we output no array or an empty array? :) what were we doing before? not sure this change is needed.

This comment has been minimized.

Copy link
@cbuescher

cbuescher Jun 2, 2016

Author Member

Right, that wasn't supposed to be in the commit, my bad. I will delete it.

@javanna

This comment has been minimized.

Copy link
Member

commented Jun 2, 2016

I left a few more comments on your newly added commits, but still looks good ;)

@cbuescher cbuescher force-pushed the cbuescher:remove-empty-querybuilder branch Jun 2, 2016

@cbuescher cbuescher force-pushed the cbuescher:remove-empty-querybuilder branch to 9067407 Jun 2, 2016

@cbuescher

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2016

@javanna I adressed the last two comments and pushed an update of the last commit.

@javanna

This comment has been minimized.

Copy link
Member

commented Jun 2, 2016

LGTM

@cbuescher

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2016

@javanna thanks, will merge this then.

@cbuescher cbuescher merged commit b6f26c0 into elastic:master Jun 2, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

javanna added a commit to javanna/elasticsearch that referenced this pull request Dec 12, 2016

Remove support for empty queries
Our query DSL supports empty queries (`{}`), which have a different meaning depending on the query that holds it, either ignored, match_all or match_none. We deprecated the support for empty queries in 5.0, where we log a deprecation warning wherever they are used.

The way we supported it once we moved query parsing to the coordinating node was having an Optional<QueryBuilder> return type in all of our parse methods (called fromXContent). See elastic#17624. The central place for this was QueryParseContext#parseInnerQueryBuilder. We can now remove all the optional return types and simply throw an exception whenever an empty query is found.

javanna added a commit that referenced this pull request Dec 12, 2016

Remove support for empty queries (#22092)
Our query DSL supports empty queries (`{}`), which have a different meaning depending on the query that holds it, either ignored, match_all or match_none. We deprecated the support for empty queries in 5.0, where we log a deprecation warning wherever they are used.

The way we supported it once we moved query parsing to the coordinating node was having an Optional<QueryBuilder> return type in all of our parse methods (called fromXContent). See #17624. The central place for this was QueryParseContext#parseInnerQueryBuilder. We can now remove all the optional return types and simply throw an exception whenever an empty query is found.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.