Skip to content

Conversation

cbuescher
Copy link
Member

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

PR goes agains query-refactoring feature branch.

public Query toQuery(QueryParseContext parseContext) throws QueryParsingException, IOException {
if (filters.isEmpty()) {
// no filters provided, this should be ignored upstream
return null;
Copy link
Member

Choose a reason for hiding this comment

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

same as comments in the previous PR, should we be ever returning null here? I tend to think we shouldn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as in #11629, although unfortunate, this seems necessary at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

ok

@javanna
Copy link
Member

javanna commented Jun 15, 2015

left two minor comments, LGTM besides those

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#11628
@cbuescher cbuescher force-pushed the feature/query-refactoring-and branch from 146587f to 5a16ddc Compare June 16, 2015 12:17
@cbuescher
Copy link
Member Author

Renamed the test case, keeping the null return value for reasons statet above.

@javanna
Copy link
Member

javanna commented Jun 16, 2015

LGTM

cbuescher added a commit that referenced this pull request Jun 16, 2015
Query refactoring: AndQueryBuilder and Parser
@cbuescher cbuescher merged commit 22cf442 into elastic:feature/query-refactoring Jun 16, 2015
@kevinkluge kevinkluge removed the review label Jun 16, 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#11628
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
…ring-and

Query refactoring: AndQueryBuilder and Parser
@cbuescher cbuescher deleted the feature/query-refactoring-and branch March 31, 2016 09:26
@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