Merge NamedWriteable changes from master #12694
Merged
Conversation
This file contains 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
…lders and QueryParsers The planed refactoring of search queries layed out in elastic#9901 requires to split the "parse()" method in QueryParsers into two methods, first a "fromXContent(...)" method that allows parsing to an intermediate query representation (currently called FooQueryBuilder) and second a "Query toQuery(...)" method on these intermediate representations that create the actual lucene queries. This PR is a first step in that direction as it introduces the interface changes necessary for the further refactoring. It introduces the new interface methods while for now keeping the old Builder/Parsers still in place by delegating the new "toQuery()" implementations to the existing "parse()" methods, and by introducing a "catch-all" "fromXContent()" implementation in a BaseQueryParser that returns a temporary QueryBuilder wrapper implementation. This allows us to refactor the existing QueryBuilders step by step while already beeing able to start refactoring queries with nested inner queries. Closes elastic#10580
Conflicts: src/main/java/org/elasticsearch/index/query/IdsQueryParser.java
Conflicts: src/main/java/org/elasticsearch/index/query/CommonTermsQueryBuilder.java src/main/java/org/elasticsearch/index/query/GeoShapeQueryParser.java src/main/java/org/elasticsearch/index/query/HasParentQueryParser.java src/main/java/org/elasticsearch/index/query/QueryBuilder.java src/main/java/org/elasticsearch/index/query/functionscore/FunctionScoreQueryBuilder.java
…Builders and FilterParsers The planned refactoring of search queries requires to split the "parse()" method in FilterParsers into two methods, first a "fromXContent(...)" method that allows parsing to an intermediate filter representation (currently called FooFilterBuilder) and second a "Filter toFilter(...)" method on these intermediate representations that create the actual lucene filters. This PR is a first step in that direction as it introduces the interface changes necessary for the further refactoring. It introduces the new interface methods while for now keeping the old Builder/Parsers still in place by delegating the new "toFilter()" implementations to the existing "parse()" methods, and by introducing a "catch-all" "fromXContent()" implementation in a BaseFilterParser that returns a temporary FilterBuilder wrapper implementation. This allows us to refactor the existing FilterBuilders step by step while already being able to start refactoring queries and filters with inner queries/filters.
…nce we move to parsing search requests on the coordinating node
Conflicts: src/main/java/org/elasticsearch/index/query/FilterBuilder.java src/main/java/org/elasticsearch/index/query/QueryParseContext.java
…der/Parser merged in from master to query refactoring base clases.
Also extended BaseQueryTestCase so it has helper methods for parsing the query header and extended the toQuery() test method so it passes down parse context to sublass to make assertions on side effects calling toQuery() has on the parseContext. Added validate() method to QueryBuilder and BaseQueryBuilder that can be used to validate a query and that returns a QueryValidationException containing all collected validation errors.
We are creating random queries for testing purposes in BaseQueryTestCase and discussed using more repetitions of the basic test cases (e.g. serialization, fromXContent parsing) for better coverage if the query has more options. Closes elastic#11021
…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
Conflicts: src/main/java/org/elasticsearch/index/query/ConstantScoreQueryBuilder.java src/main/java/org/elasticsearch/index/query/FilterBuilder.java src/main/java/org/elasticsearch/index/query/FilterParser.java src/main/java/org/elasticsearch/index/query/GeoShapeQueryBuilder.java src/main/java/org/elasticsearch/index/query/QueryParseContext.java src/main/java/org/elasticsearch/index/query/TermQueryParser.java src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java
Split the parse(QueryParseContext ctx) method into a parsing and a query building part, adding Streamable for serialization and hashCode(), equals() for better testing. Add basic unit test for Builder and Parser. Closes elastic#10670
…aseQueryTestCase
…validate Following up to elastic#12427, this PR does same changes, moving null-checks from construtors and setters in query builder to the validate() method. PR against query-refactoring branch
…ring-nullCheckCleanup Query Refactoring: Move null-checks to validate
If we cache the type filter and we e.g. set its boost which is now settable on all queries, the boost will change for all subsequent queries. We should rather create a new query every time.
…type_query Query DSL: don't cache type filter in DocumentMapper
core/src/test/java/org/elasticsearch/aliases/IndexAliasesTests.java
We currently test the toQuery method by comparing its return value with the output of createExpectedQuery. This seemed at first like a good deep test for the lucene generated queries, but ends up causing a lot of code duplication between tests and prod code, which smells like this test is not that useful after all. This commit removes the requirement for createExpectedQuery, we test a bit less the lucene generated queries, but we do still have a testToQuery method in BaseQueryTestCase. This test acts as smoke test to verify that there are no issues with our toQuery method implementation for each query: it verifies that two equal queries should result in the same lucene query. It also verifies that changing the boost value of a query affects the result of the toQuery. Part of the previous createExpectedQuery can be moved to assertLuceneQuery using normal assertions, we do it when it makes sense only though. This commit also adds support for boost and queryName to SpanMultiTermQueryParser and SpanMultiTermQueryBuilder, plus it removes no-op setters for queryName and boost from QueryFilterBuilder. Instead we simply declare in our test infra that query filter doesn't support boost and queryName and we disable tests aroudn these fields in that specific case. Boost and queryName support is also moved down to the QueryBuilder interface. Closes elastic#12473
Conflicts: core/src/test/java/org/elasticsearch/common/io/streams/BytesStreamsTests.java core/src/test/java/org/elasticsearch/search/highlight/HighlighterSearchIT.java core/src/test/java/org/elasticsearch/search/query/SearchQueryIT.java core/src/test/java/org/elasticsearch/test/transport/AssertingLocalTransport.java
We are currently splitting the parse() method in the query parsers into a query parsing part (fromXContent(QueryParseContext)) and a new method that generates the lucene query (toQuery(QueryParseContext)). At some point we would like to be able to excute these two steps in different phases, one on the coordinating node and one on the shards. This PR tries to anticipate this by renaming the existing QueryParseContext to QueryShardContext to make it clearer that this context object will provide the information needed to generate the lucene queries on the shard. A new QueryParseContext is introduced and all the functionality needed for parsing the XContent in the request is moved there (parser, helper methods for parsing inner queries). While the refactoring is not finished, the new QueryShardContext will expose the QueryParseContext via the parseContext() method. Also, for the time beeing the QueryParseContext will contain a link back to the QueryShardContext it wraps, which eventually will be deleted once the refactoring is done. Closes elastic#12527 Relates to elastic#10217
…ring-queryCreationContext Separating QueryParseContext and QueryShardContext
Conflicts: core/src/main/java/org/elasticsearch/transport/local/LocalTransport.java
Conflicts: core/src/main/java/org/elasticsearch/common/io/stream/FilterStreamInput.java core/src/main/java/org/elasticsearch/common/io/stream/NamedWriteableRegistry.java core/src/main/java/org/elasticsearch/common/io/stream/StreamInput.java core/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java core/src/main/java/org/elasticsearch/transport/netty/MessageChannelHandler.java core/src/test/java/org/elasticsearch/common/io/stream/BytesStreamsTests.java core/src/test/java/org/elasticsearch/transport/AbstractSimpleTransportTests.java
| @@ -111,7 +111,7 @@ public QueryValidationException validate() { | |||
| } | |||
|
|
|||
| @Override | |||
| public String getName() { | |||
| public String getWriteableName() { | |||
There was a problem hiding this comment.
I got sidetracked while wondering if we should make all these methods final so no subclass can change the name. Now I'm unsure how subclassing queries would work in terms of name, writeTo, but especially readFrom(). Suppose we have a SpecialAndQueryBuilder extends AndQueryBuilder, having its own WritableName is okay, also doWriteTo could call super.doWriteTo(), but as far as I can see we'd have to repeat the readFrom code in any subclass since the superclass always return their own object type. Or is this something we just assume we won't have to do for other NamedWritables?
There was a problem hiding this comment.
For the agg refactoring I am doing the following: https://github.com/colings86/elasticsearch/blob/enhancement/minAggRefactor/core/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregatorFactory.java#L306 basically the super class defines a template method which is implemented by the sub-class to create the instance and then the super-class can call the methods it needs on that object to set what it needs to.
There was a problem hiding this comment.
I think we don't have this case for queries, cause when we subclass the query usually the base class does everything around serialization. If we did have the case though, like probably aggs do, I wouldn't make this method final and yes we would have to work around the readFrom problem as Colin suggested. Don't see how we can work around it otherwise.
|
@javanna I had a look at only the changes in the QueryBuilder hierarchy, looks good to me. Left one general comment, just a question that came to mind in general for clarification. |
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.
This PR merges recent changes from master in our query-refactoring branch. It includes changes around NamedWriteable from #12571.
Sending it as a PR so people can have a look at it, see the impact of the changes and the changes required on our side before stuff gets in.