Skip to content

Query refactoring: NotQueryBuilder and Parser#11823

Merged
cbuescher merged 1 commit intoelastic:feature/query-refactoringfrom
cbuescher:feature/query-refactoring-not
Jun 23, 2015
Merged

Query refactoring: NotQueryBuilder and Parser#11823
cbuescher merged 1 commit intoelastic:feature/query-refactoringfrom
cbuescher:feature/query-refactoring-not

Conversation

@cbuescher
Copy link
Copy Markdown
Member

Moving the query building functionality from the parser to the builders
new toQuery() method analogous to other recent query refactorings.

Relates to #10217
Closes #11823

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't this convert to an if in validate?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think so, unfortunately parseInnerFilter/Builder() can legally return null and also does this when the inner query e.g. is { "constant_score" : filter {} }. This is currently legal rest query which parses to null in perseInnerXxx(), this is then passed up by enclosing filters/queries until it gets ignored somewhere.
The former existing null-check was okay for the existing builder-pattern, but now that we use it for intermediate query representation as well, I think this needs to be allowed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

makes sense to me, thanks for the explanation, I see what you mean

@javanna
Copy link
Copy Markdown
Contributor

javanna commented Jun 23, 2015

left a small comment, LGTM otherwise

Moving the query building functionality from the parser to the builders
new toQuery() method analogous to other recent query refactorings.

Relates to elastic#10217
Closes elastic#11823
@cbuescher cbuescher force-pushed the feature/query-refactoring-not branch from 2609956 to 682b499 Compare June 23, 2015 11:47
cbuescher added a commit that referenced this pull request Jun 23, 2015
Query refactoring: NotQueryBuilder and Parser
@cbuescher cbuescher merged commit 0da9f20 into elastic:feature/query-refactoring Jun 23, 2015
@kevinkluge kevinkluge removed the review label Jun 23, 2015
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
Moving the query building functionality from the parser to the builders
new toQuery() method analogous to other recent query refactorings.

Relates to elastic#10217
Closes elastic#11823
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
…ring-not

Query refactoring: NotQueryBuilder and Parser
@cbuescher cbuescher deleted the feature/query-refactoring-not branch March 31, 2016 09:27
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query Refactoring labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Search/Search Search-related issues that do not fall into other categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants