Query Refactoring: ExistsQueryBuilder and Parser #11427

cbuescher · 2015-05-29T16:12:07Z

Refactors ExistsQueryBuilder and Parser, splitting the parse() method into a parsing and a query building part. Also moving newFilter() uitlity method from parser to query builder.

Changes in the BaseQueryTestCase include introduction of randomized version to test disabled FieldNamesFieldMappers and also getting rid of the need for createEmptyBuilder() method by now using existing prototype constants.

Relates to #10217

PR goes agains query-refacoring feature branch.

cbuescher · 2015-05-29T16:17:57Z

I'm still not 100% happy with the test for the query generation (toQuery) since constructing the expected Lucene query relies on using newFilter() helper method. Testing that helper method itself would require a lot of repetition of inner implementation logic in the creating of the expected query, so I choose not to go into that for now. Since it's a static method, maybe that would be better handled in a separate integration test?

MaineC · 2015-06-03T07:27:58Z

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


 /**
 *
 */
-public class ExistsQueryParser extends BaseQueryParserTemp {
+public class ExistsQueryParser extends BaseQueryParser {

    @Inject


Is the annotation needed on a constructor w/o arguments?

I think so, at least we have it on a couple of other no-arg parsers. I'll try what happens when I remove them but I guess it messes up some guice registry stuff.

cbuescher · 2015-06-04T17:09:21Z

Extended test for the toQuery() part of the ExistsQueryBuilder. Setup now randomly disables FieldNamesFieldMapper so we cover more execution paths.

cbuescher · 2015-06-04T19:16:40Z

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

+        final FieldNamesFieldMapper fieldNamesMapper = (FieldNamesFieldMapper)parseContext.mapperService().fullName(FieldNamesFieldMapper.NAME);
+
+        MapperService.SmartNameObjectMapper smartNameObjectMapper = parseContext.smartObjectMapper(fieldPattern);
+        if (smartNameObjectMapper != null && smartNameObjectMapper.hasMapper()) {


I couldn't find out yet how to test this branch. I'll try to find out what it actually does and find a way to insert SmartNameObjectMapper into parseContext in out setup.

Changed test setup with recent commit so this is covered now.

javanna · 2015-06-05T14:54:02Z

src/test/java/org/elasticsearch/index/query/BaseQueryTestCase.java

@@ -266,7 +284,10 @@ public void testSerialization() throws IOException {
     * @return a new {@link QueryParseContext} based on the base test index and queryParserService
     */
    protected static QueryParseContext createContext() {
-        return new QueryParseContext(index, queryParserService);
+        QueryParseContext context = new QueryParseContext(index, queryParserService);
+        QueryParseContext.setTypes(currentTypes);


mmm don't we have this somewhere else already? being it static, we shouldn't do it every time we create a new context, only once

Right, I missed that we have similar setup in beforeTest(). Reverted this.

javanna · 2015-06-05T14:56:35Z

looks good, left a few comments

cbuescher · 2015-06-08T14:06:05Z

@javanna Thanks, adressed your last comments and rebased on current head of feature branch.

javanna · 2015-06-09T14:31:05Z

core/src/main/java/org/elasticsearch/index/query/ExistsQueryBuilder.java

@@ -41,6 +54,13 @@ public ExistsQueryBuilder(String name) {
    }

    /**
+     * @return the field name that has to exists for this query to match


s/exists/exist

javanna · 2015-06-09T15:52:00Z

I left a couple of minor comments, LGTM though, feel free to push once you addressed those

cbuescher · 2015-06-10T10:52:48Z

Rebasing on current head of feature branch and addressing last round of comments.
I needed to adapt the BaseQueryTest setup a bit to conform with current changes merged in from master concerning Mappings, especially FieldNameFieldMapper. See inline comment for explication.

cbuescher · 2015-06-10T10:58:18Z

core/src/test/java/org/elasticsearch/index/query/BaseQueryTestCase.java

@@ -125,7 +135,7 @@ protected void configure() {
        queryParserService = injector.getInstance(IndexQueryParserService.class);
        MapperService mapperService = queryParserService.mapperService;
        //create some random type with some default field, those types will stick around for all of the subclasses
-        currentTypes = new String[randomIntBetween(0, 5)];
+        currentTypes = new String[randomIntBetween(1, 5)];


I temporarily changed this to add at least one type plus mappings because so far in our test setup the FieldNamesFieldMapper which we now need to be non-null in ExistQueryBuilder is initiallized only when triggered by the mapperService.merge. Before there were null checks in the ExistQueryParser/Builder newFilter() method that now got removed on master. Still looking for other ways to change the test setup here.

It kinda makes sense to me that the field is not there if we have no types (aka the index is empty). This is exactly why I want to keep testing against no types. I think this is a bug introduced with #11559 where a null check was removed (1fdae75#commitcomment-11609013). The FieldNamesFieldMapper can be null and we should handle such situation properly. Once that gets fixed upstream our tests can still use no types randomly I think.

javanna · 2015-06-10T11:51:14Z

left two comments, LGTM though. We have to resolve upstream the problem around FieldNamesFieldMapper and we are good to go I'd say.

cbuescher · 2015-06-10T12:53:21Z

Added randomization of the version in settings, will wait hearing back about possible upstream changes.

…text ctx) method into a parsing and a query building part, adding NamedWriteable implementation for serialization and hashCode(), equals() for testing. This change also adds tests using fixed set of leaf queries (Terms, Ids, MatchAll) as nested Queries in test query setup. Also QueryParseContext is adapted to return QueryBuilder instances for inner filter parses instead of previously returning Query instances, keeping old methods in place but deprecating them. Relates to elastic#10217 Closes elastic#11427

Refactors ExistsQueryBuilder and Parser, splitting the parse() method into a parsing and a query building part. Also moving newFilter() utility method from parser to query builder. Changes in the BaseQueryTestCase include introduction of randomized version to test disabled FieldNamesFieldMappers and also getting rid of the need for createEmptyBuilder() method by using registered prototype constants. Relates to elastic#10217 Closes elastic#11427

javanna · 2015-06-11T12:44:45Z

LGTM

…ists Query Refactoring: ExistsQueryBuilder and Parser

…text ctx) method into a parsing and a query building part, adding NamedWriteable implementation for serialization and hashCode(), equals() for testing. This change also adds tests using fixed set of leaf queries (Terms, Ids, MatchAll) as nested Queries in test query setup. Also QueryParseContext is adapted to return QueryBuilder instances for inner filter parses instead of previously returning Query instances, keeping old methods in place but deprecating them. Relates to elastic#10217 Closes elastic#11427

Refactors ExistsQueryBuilder and Parser, splitting the parse() method into a parsing and a query building part. Also moving newFilter() utility method from parser to query builder. Changes in the BaseQueryTestCase include introduction of randomized version to test disabled FieldNamesFieldMappers and also getting rid of the need for createEmptyBuilder() method by using registered prototype constants. Relates to elastic#10217 Closes elastic#11427

…ring-exists Query Refactoring: ExistsQueryBuilder and Parser

cbuescher added :Query Refactoring labels May 29, 2015

MaineC reviewed Jun 3, 2015
View reviewed changes

cbuescher force-pushed the feature/query-refactoring-exists branch 2 times, most recently from 3febb94 to dfa90fa Compare June 4, 2015 11:38

cbuescher reviewed Jun 4, 2015
View reviewed changes

cbuescher force-pushed the feature/query-refactoring-exists branch from dfa90fa to 46540d5 Compare June 5, 2015 13:21

javanna reviewed Jun 5, 2015
View reviewed changes

cbuescher force-pushed the feature/query-refactoring-exists branch from 46540d5 to e0393da Compare June 8, 2015 14:02

cbuescher force-pushed the feature/query-refactoring-exists branch from e0393da to ed335b2 Compare June 8, 2015 15:19

javanna reviewed Jun 9, 2015
View reviewed changes

cbuescher force-pushed the feature/query-refactoring-exists branch from ed335b2 to 10a3fe1 Compare June 10, 2015 10:50

cbuescher reviewed Jun 10, 2015
View reviewed changes

cbuescher force-pushed the feature/query-refactoring-exists branch from 97900c9 to 6700261 Compare June 11, 2015 11:40

cbuescher force-pushed the feature/query-refactoring-exists branch from 6700261 to c928852 Compare June 11, 2015 12:41

cbuescher added a commit that referenced this pull request Jun 11, 2015

Merge pull request #11427 from cbuescher/feature/query-refactoring-ex…

bd2e163

…ists Query Refactoring: ExistsQueryBuilder and Parser

cbuescher merged commit bd2e163 into elastic:feature/query-refactoring Jun 11, 2015

kevinkluge removed the review label Jun 11, 2015

clintongormley mentioned this pull request Jun 23, 2015

Refactor parsing of queries/filters, aggs, suggester APIs #10217

Closed

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

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

d481bad

…ring-exists Query Refactoring: ExistsQueryBuilder and Parser

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-exists branch March 20, 2024 20:15

Query Refactoring: ExistsQueryBuilder and Parser #11427

Query Refactoring: ExistsQueryBuilder and Parser #11427

Uh oh!

Conversation

cbuescher commented May 29, 2015

Uh oh!

cbuescher commented May 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Jun 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Jun 5, 2015

Uh oh!

cbuescher commented Jun 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Jun 9, 2015

Uh oh!

cbuescher commented Jun 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Jun 10, 2015

Uh oh!

cbuescher commented Jun 10, 2015

Uh oh!

javanna commented Jun 11, 2015

Uh oh!

Uh oh!