Query Refactoring: Moving parser NAME constant to corresponding query builder by cbuescher · Pull Request #11178 · elastic/elasticsearch

cbuescher · 2015-05-15T08:32:19Z

As part of the query refactoring, this PR moves the NAME field that currently identifies each query parser to the corresponding query builder. The reason for this, as discussed in #11121 (comment), is that we need still need to be able to link parser and builder implementations, but that the query builder is independent of the parser (queries don't necessarily need to be coverted to XContent any more). Builders don't need to know about their parsers, but parsers need to be linked to their respective builders.

This change does the following:

move the NAME constants from the parsers to the builders, making sure they are unique
rename the existing getParserName() in the Builder classes to queryId(), returning the unique NAME constant. This is now public so we can retrieve query name when coding against the QueryBuilder abstract class
modify the names() method in the parsers so it contains at least one member that is the query id

cbuescher · 2015-05-15T08:42:03Z

As a further explanation: there were two cases where the 1:1 mapping from parsers to builders was not completely clear:

FQueryFilterParser didn't have a corresponding Builder class, instead queries with the "fquery" id were build in the (already deprecated) QueryFilterBuilder. To make this link clearer I introduced a new FQueryFilterParser that extends QueryFilterBuilder but otherwise shares the existing code there, just carries the moved NAME constant
TermsLookupQueryBuilder was a builder without a correspondingly named parser. It used the TermsQueryParser and so also its NAME. To have a unique id for each builder I introduced the new name "terms_lookup"` here and added this to the names() array in the TermsQueryParser. Because all these names are used to register the parsers this will link the builder to that parser now.

javanna · 2015-05-15T08:45:07Z

src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java

in all the builders, you can remove the class name, just return NAME is enough.

javanna · 2015-05-15T10:01:29Z

good change! left a few comments

cbuescher · 2015-05-21T16:40:10Z

I rebased this PR after recent changes on feature branch and addressed the two comments above. The two main changes here:

TermsQueryBuilder covers all of former TermsLookupBuilder functionality, leaving deprecated Stub class, leading to a 1:1 relation between Builder and Parser here.
FQueryFilterBuilder directly extends QueryBuilder, moved cases where queryName is set out of QueryFilterBuilder.

javanna · 2015-05-21T19:57:22Z

src/main/java/org/elasticsearch/index/query/QueryFilterBuilder.java

I thought we discussed (missing bits in the review comments) to leave this as-is and to not introduce FQueryFilterBuilder as it is not needed. This change breaks bw compatibility as you pointed out, thus we should keep this class as-is, and if we do that it makes little sense to introduce a deprecated additional FQueryFilterBuilder. The idea is that two parsers sharing the same builder should not be a problem....unless I am missing something.

All right, will remove FQueryFilterBuilder then, put the NAME of the extra FQueryFilterParser somewhere else in the QueryFilterBuilder then.

javanna · 2015-05-21T20:03:29Z

I did another review round, thanks @cbuescher

cbuescher · 2015-05-22T11:04:16Z

@javanna I had another round, mostly reverting some of the changes in my prevoius commits.

javanna · 2015-05-22T11:19:57Z

src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java

too many tabs?

javanna · 2015-05-22T11:21:42Z

left a super minor comment on indentation, LGTM besides that

… builder As part of the query refactoring, this PR moves the NAME field that currently identifies each query parser to the corresponding query builder. The reason for this, as discussed in elastic#11121 (comment), is that we need still need to be able to link parser and builder implementations, but that the query builder is independent of the parser (queries don't necessarily need to be coverted to XContent any more). Builders don't need to know about their parsers, but parsers need to be linked to their respective builders. Closes elastic#11178

…vename Query Refactoring: Moving parser NAME constant to corresponding query builder

… builder As part of the query refactoring, this PR moves the NAME field that currently identifies each query parser to the corresponding query builder. The reason for this, as discussed in elastic#11121 (comment), is that we need still need to be able to link parser and builder implementations, but that the query builder is independent of the parser (queries don't necessarily need to be coverted to XContent any more). Builders don't need to know about their parsers, but parsers need to be linked to their respective builders. Closes elastic#11178

…ring-movename Query Refactoring: Moving parser NAME constant to corresponding query builder

javanna reviewed May 15, 2015
View reviewed changes

javanna added the :Query Refactoring label May 21, 2015

cbuescher force-pushed the feature/query-refactoring-movename branch from 127195f to 59305dc Compare May 21, 2015 16:30

javanna reviewed May 21, 2015
View reviewed changes

javanna reviewed May 22, 2015
View reviewed changes

src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java Outdated

Copy link
Copy Markdown

Contributor

javanna May 22, 2015

Choose a reason for hiding this comment

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

too many tabs?

cbuescher force-pushed the feature/query-refactoring-movename branch from f5a4e74 to 283fe90 Compare May 22, 2015 12:07

cbuescher added a commit that referenced this pull request May 22, 2015

Merge pull request #11178 from cbuescher/feature/query-refactoring-mo…

1c60436

…vename Query Refactoring: Moving parser NAME constant to corresponding query builder

cbuescher merged commit 1c60436 into elastic:feature/query-refactoring May 22, 2015

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015

Merge pull request elastic#11178 from cbuescher/feature/query-refacto…

123f49b

…ring-movename Query Refactoring: Moving parser NAME constant to corresponding query builder

clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query Refactoring labels Feb 14, 2018

cbuescher deleted the feature/query-refactoring-movename branch March 20, 2024 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query Refactoring: Moving parser NAME constant to corresponding query builder#11178

Query Refactoring: Moving parser NAME constant to corresponding query builder#11178
cbuescher merged 1 commit intoelastic:feature/query-refactoringfrom
cbuescher:feature/query-refactoring-movename

cbuescher commented May 15, 2015

Uh oh!

cbuescher commented May 15, 2015

Uh oh!

javanna May 15, 2015

Uh oh!

javanna commented May 15, 2015

Uh oh!

cbuescher commented May 21, 2015

Uh oh!

javanna May 21, 2015

Uh oh!

cbuescher May 22, 2015

Uh oh!

javanna commented May 21, 2015

Uh oh!

cbuescher commented May 22, 2015

Uh oh!

javanna May 22, 2015

Uh oh!

javanna commented May 22, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

cbuescher commented May 15, 2015

Uh oh!

cbuescher commented May 15, 2015

Uh oh!

javanna May 15, 2015

Choose a reason for hiding this comment

Uh oh!

javanna commented May 15, 2015

Uh oh!

cbuescher commented May 21, 2015

Uh oh!

javanna May 21, 2015

Choose a reason for hiding this comment

Uh oh!

cbuescher May 22, 2015

Choose a reason for hiding this comment

Uh oh!

javanna commented May 21, 2015

Uh oh!

cbuescher commented May 22, 2015

Uh oh!

javanna May 22, 2015

Choose a reason for hiding this comment

Uh oh!

javanna commented May 22, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants