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.

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

Jump to bottom

Remove filter parsers. #10985

Merged
merged 1 commit into from
May 7, 2015
Merged

Remove filter parsers. #10985

merged 1 commit into from
May 7, 2015

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented May 5, 2015

This commit makes queries and filters parsed the same way using the
QueryParser abstraction. This allowed to remove duplicate code that we had
for similar queries/filters such as range, prefix or term.

@jpountz
Copy link
Contributor Author

jpountz commented May 5, 2015

For the record, the main challenges here were to deal with filters that behave differently from their equivalent query:

  • bool has a default min_should_match of 0 in queries and 1 in filters that have should clauses
  • terms parses as a boolean query when used as a query and a terms query otherwise

The query DSL is backward compatible, existing queries should keep working. Actually, it even allows to eg. use the script query by itself without wrapping it into a constant_score or filtered_query.

Regarding documentation, I moved query definitions right under query-dsl while it used to be splitted into queries and filters.

@jpountz
Copy link
Contributor Author

jpountz commented May 5, 2015

The diff is very large due to the fact that we have a lot of code that manipulates queries/filters but not very interesting on average, it might be easier to just ask questions. :-)

rmuir
rmuir reviewed May 5, 2015
View reviewed changes
docs/reference/query-dsl/exists-query.asciidoc
@@ -41,6 +41,9 @@ These documents would *not* match the above filter:
<2> At least one non-`null` value is required.
<3> The `user` field is missing completely.

The scores produced by this query should typically not be relied upon. It is
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the confusing warning? tf values are always 1, so in the common case it just adds some delta to every document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it.

@clintongormley
Copy link
Contributor

In the constant_score query, I think we should deprecate the query parameter, because anything under there will be treated as a filter.

clintongormley
clintongormley reviewed May 6, 2015
View reviewed changes
docs/reference/query-dsl/bool-query.asciidoc
@@ -22,6 +22,9 @@ parameter.
documents.
|=======================================================================

If this query is used in a filter context and it has `should` clauses then at
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a big IMPORTANT clause

@jpountz
Copy link
Contributor Author

jpountz commented May 7, 2015

I pushed more commits to address comments.

I'm also adding the breaking label because users of the Java API will need to change their code to use QueryBuilders instead of FilterBuilders. For users of the REST API however, this change is backward compatible.

@javanna
Copy link
Member

javanna commented May 7, 2015

All good on my end, thanks for updating @jpountz . I don't have enough lucene knowledge though to review the lucene aspects here.

javanna added a commit that referenced this pull request May 7, 2015
@javanna
Revert "Filter refactoring: Introduce toFilter() and fromXContent() i…
0dbffbe 
…n FilterBuilders and FilterParsers"

This reverts commit 580ef6f given that FilterBuilder and FilterParser are going away with #10985

Conflicts:
	src/main/java/org/elasticsearch/index/query/BaseFilterBuilder.java
	src/main/java/org/elasticsearch/index/query/BaseFilterParser.java
	src/main/java/org/elasticsearch/index/query/BaseFilterParserTemp.java
	src/main/java/org/elasticsearch/index/query/FilterWrappingFilterBuilder.java
javanna added a commit that referenced this pull request May 7, 2015
@javanna
removed //norelease comment, FilterParser will go away with #10985
ed094aa
@javanna javanna mentioned this pull request May 7, 2015
Merged
@jpountz jpountz force-pushed the enhancement/remove_filters branch from d49a4d8 to da66ccc Compare May 7, 2015 18:13
@jpountz
Query DSL: Remove filter parsers.
a0af88e 
This commit makes queries and filters parsed the same way using the
QueryParser abstraction. This allowed to remove duplicate code that we had
for similar queries/filters such as `range`, `prefix` or `term`.
@jpountz jpountz force-pushed the enhancement/remove_filters branch from da66ccc to a0af88e Compare May 7, 2015 18:14
@jpountz jpountz removed the review label May 7, 2015
jpountz added a commit that referenced this pull request May 7, 2015
@jpountz
Merge pull request #10985 from jpountz/enhancement/remove_filters
e7540e9 
Query DSL: Remove filter parsers.
@jpountz jpountz merged commit e7540e9 into elastic:master May 7, 2015
@javanna javanna mentioned this pull request May 8, 2015
Closed
jpountz added a commit to jpountz/elasticsearch that referenced this pull request May 12, 2015
@jpountz
Query DSL: Fix bool parsing.
caf0286 
In elastic#10985 I introduced a bug that should clauses are parsed as filters while
must_not clauses should be parsed as filters.
@jpountz jpountz mentioned this pull request May 12, 2015
Merged
jpountz added a commit to jpountz/elasticsearch that referenced this pull request May 13, 2015
@jpountz
Query DSL: Fix bool parsing.
11db317 
In elastic#10985 I introduced a bug that should clauses are parsed as filters while
must_not clauses should be parsed as filters.
@clintongormley clintongormley changed the title Query DSL: Remove filter parsers. Remove filter parsers. Jun 6, 2015
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
@javanna
Revert "Filter refactoring: Introduce toFilter() and fromXContent() i…
79aa617 
…n FilterBuilders and FilterParsers"

This reverts commit 580ef6f given that FilterBuilder and FilterParser are going away with elastic#10985

Conflicts:
	src/main/java/org/elasticsearch/index/query/BaseFilterBuilder.java
	src/main/java/org/elasticsearch/index/query/BaseFilterParser.java
	src/main/java/org/elasticsearch/index/query/BaseFilterParserTemp.java
	src/main/java/org/elasticsearch/index/query/FilterWrappingFilterBuilder.java
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
@javanna
removed //norelease comment, FilterParser will go away with elastic#1…
12ee89f 
…0985
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query DSL labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
>breaking >enhancement :Search/Search Search-related issues that do not fall into other categories v2.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jpountz @clintongormley @javanna @rmuir