Query Refactoring: Adding getBuilderPrototype() method to all QueryParsers #11344
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
I was wondering if we should use some convention on naming, like INSTANCE or PROTOTYPE, I have no strong preference though
|
looks good, left a few comments |
cbuescher
force-pushed
the
feature/query-refactoring-linkParsers
branch
from
f876767 to
2a28587
Compare
|
@javanna had another round, moved the constants to the builders and reverted most of the package private constructors, only introduced private constructors where needed. |
|
LGTM thanks @cbuescher |
…rsers Currently there is a registry for all QueryParsers accessible via the IndicesQueriesModule. For deserializing nested queries e.g. for the BoolQueryBuilder (elastic#11121) we need to look up query builders by their name to be able to deserialize using a prototype builder of the concrete class. This PR adds a getBuilderPrototype() method to each query parser so we can re-use the parser registry to get the corresponding builder using the query name. Closes elastic#11344
cbuescher
force-pushed
the
feature/query-refactoring-linkParsers
branch
from
e054e23 to
8a37cc2
Compare
|
Thanks, rebased and will merge then. |
cbuescher
added a commit
that referenced
this pull request
May 26, 2015
…nkParsers Query Refactoring: Adding getBuilderPrototype() method to all QueryParsers
|
I am marking this as breaking, as it breaks plugins that introduce custom queries by requiring their |
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
May 26, 2015
… queries `QueryBuilder`s can be serialized being them `Writeable`. Given that we support different types of queries, which can be nested into one another, we also have to make sure that we read and write the name of the query before the actual query fields so that we know which instance of query we have to create when de-serializing. That is why serializing a query builder will be done by calling `QueryBuilderSerializer#write`, which will serialize the name of the query before the query itself, and reading only by calling `QueryBuilderSerializer#read`, which will lookup the query in the parsers registry, retrieve its corresponding empty builder from it (`getPrototypeBuilder`) and call `readFrom` against it, which will return a new query builder containing all the de-serialized fields. Compound queries will then have to use these methods to read and write their inner queries. The registration of custom queries via plugin doesn't change, only the parser needs to be registered still, but it has now to implement the `getPrototypeBuilder` method too introduced with elastic#11344, which returns an empy builder instance that can be used to de-serialize the query.
mute
pushed a commit
to mute/elasticsearch
that referenced
this pull request
Jul 29, 2015
…rsers Currently there is a registry for all QueryParsers accessible via the IndicesQueriesModule. For deserializing nested queries e.g. for the BoolQueryBuilder (elastic#11121) we need to look up query builders by their name to be able to deserialize using a prototype builder of the concrete class. This PR adds a getBuilderPrototype() method to each query parser so we can re-use the parser registry to get the corresponding builder using the query name. Closes elastic#11344
mute
pushed a commit
to mute/elasticsearch
that referenced
this pull request
Jul 29, 2015
…ring-linkParsers Query Refactoring: Adding getBuilderPrototype() method to all QueryParsers
clintongormley
added
:Search/Search
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently there is a registry for all QueryParsers accessible via the IndicesQueriesModule. For deserializing nested queries e.g. for the BoolQueryBuilder (#11121) we need to look up query builders by their name to be able to deserialize using a prototype builder of the concrete class. This PR adds a getBuilderPrototype() method to each query parser so we can re-use the parser registry to get the corresponding builder using the query name. This might change in the future when we decide to register also builders or merger parsers and builders in a later stage of the refactoring. (see #11121 (comment)) for past discussion.