Rework a query parser and improve registration #17458

nik9000 · 2016-03-31T19:39:54Z

By default queries now register using both their snake_case and camelCase
name. We no longer need to delay construction of the parser map so we can
throw out QueryRegistration and no longer need Suppliers.

nik9000 · 2016-03-31T19:41:14Z

Another one for @javanna and @cbuescher. Two more queries down out of about 45.

cbuescher · 2016-04-01T10:30:24Z

core/src/main/java/org/elasticsearch/search/SearchModule.java

@@ -310,15 +305,33 @@ public void registerFunctionScoreParser(ScoreFunctionParser<? extends ScoreFunct
     *        registered. So it is the name that the query should use as its {@link NamedWriteable#getWriteableName()}.
     */
    public <QB extends QueryBuilder<QB>> void registerQuery(Writeable.Reader<QB> reader, QueryParser<QB> parser, String... names) {


This doesn't directly concern changes in this PR, but the first name should be mandatory here. What about making this signature String queryName, String alternativeNames or something along those lines?

that is a good point Christoph! if we really want to keep varargs otherwise we could validate that the array has length >=1 , I have no strong preference on how we solve it though

I think we should make sure that the String that is used as Id in serialization gets treated in a special way, also in the builders themselves so they are not accidentally changed between versions. Thats why I liked the old getWritableName() (that seems to be on its way out).
Maybe we should even have an own class QueryId which simply wraps the NAME string constant but forces us (and users implementing their own queries) to think about this as a special case. We could change the existing String getName() method in QueryBuilder that currenty just forwards to getWritableName to do this.
This is just some thought for discussion, nothing to block this PR though.

I agree with you 100% we have to be more careful with query ids and serialization problems. With this change we hide even more that the first name is used for serialization, maybe we can already split the two (parser names and writeable id) more clearly instead, I would go that path.

cbuescher · 2016-04-01T11:17:18Z

@nik9000 I left a small comment about a missing name constant and some general thought about whether we should keep some of the query identifiers in their respective classes for discussion.

nik9000 · 2016-04-01T16:12:24Z

@cbuescher, @javanna, @colings86 and I had a video chat about this and talked about this and we were uncomfortable with the way in which we supported query alternative names and how we just relied on the first name being the getWriteableName by convention.

I added a few patches to address those concerns. Hopefully this is better now!

javanna · 2016-04-01T18:20:16Z

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

 import java.util.Collection;
 import java.util.List;
 import java.util.Objects;

+import static java.util.Collections.unmodifiableList;
+


can you remove these redundant imports please?

Sorry, sure.

javanna · 2016-04-01T18:36:40Z

thanks a lot Nik I left my random thoughts, let's see what Colin and Christoph have to say ;)

cbuescher · 2016-04-04T09:27:59Z

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

+     * Randomize the name of a query builder. Note that this can change the meaning of the query builder. Like <code>match</code> can turn
+     * into <code>match_phrase</code>.
+     */
+    private XContentBuilder randomizeName(XContentBuilder builder, QueryBuilder<?> query) throws IOException {


I like this test, however this won't catch any accidental or unknowing changes (like removing or changing the primary name for example) from any developer. I think in addition we should explicitely check the NAME/NAMES constants in each individual test and have some comments / errors that prevent any accidental changes. Like we do for enum constants in some tests already. We can add those in follow up PRs for all queries though.

javanna · 2016-04-05T14:29:55Z

@nik9000 I think after recent changes this should easily become a simple MultiMatchQueryParser rework. regarding the test for alternate query names, I think that what we have in SearchModuleTests is good enough. ping me when you have rebased and I will review.

By default queries now register using both their snake_case and camelCase name. We no longer need to delay construction of the parser map so we can throw out QueryRegistration and no longer need Suppliers.

Note that this testing only works well on queries that have been cut over to the new registration API. Fixes GeohashCellQuery to match our conventions so we can find it's names.

QueryBuilder's can go by lots of names as we were using a String[] to refer to them all. The trouble is that some of the names are special and String[] doesn't let us easilly describe or document how they are special. So this adds QueryBuilder.Names. It gives us an easy place to document how the names are special and it gives us something we can build on, if, say, we want to register some of the names as deprecated or something like that.

nik9000 · 2016-04-05T19:18:51Z

Rebased. Since #17507 incorporated the good parts of this the rebase left this just MultiMatchQueryParser rework, a few useful line wraps, and fixing a test.

Pinging @javanna as requested.

javanna · 2016-04-06T08:16:57Z

core/src/main/java/org/elasticsearch/search/SearchModule.java

@@ -326,8 +315,8 @@ public void registerFunctionScoreParser(ScoreFunctionParser<? extends ScoreFunct
    }

    /**
-     * Register a query.
-     * TODO remove this in favor of registerQuery and rename innerRegisterQueryParser
+     * Register a query via it's parser's prototype.


s/it's/its I think

javanna · 2016-04-06T08:17:22Z

left a tiny comment, LGTM though, no need for another review

nik9000 · 2016-04-06T16:40:50Z

Closed by 1afc9b7.

nik9000 assigned javanna Mar 31, 2016

nik9000 added >enhancement review :Core/Infra/Core Core issues without another label v5.0.0-alpha1 labels Mar 31, 2016

cbuescher reviewed Apr 1, 2016
View reviewed changes

nik9000 changed the title ~~Remove two query parsers and improve registration~~ Remove a query parser and improve registration Apr 1, 2016

javanna mentioned this pull request Apr 1, 2016

Remove PROTOTYPEs from ScoreFunctionBuilders #17351

Merged

javanna reviewed Apr 1, 2016
View reviewed changes

clintongormley added v5.0.0-alpha2 and removed v5.0.0-alpha1 labels Apr 4, 2016

cbuescher reviewed Apr 4, 2016
View reviewed changes

colings86 mentioned this pull request Apr 4, 2016

Splits phrase and phrase_prefix in match query into MatchPhraseQueryBuilder and MatchPhrasePrefixQueryBuilder #17508

Merged

nik9000 mentioned this pull request Apr 4, 2016

Deprecate mlt, in and geo_bbox query name shortcuts #17507

Merged

nik9000 changed the title ~~Remove a query parser and improve registration~~ Rework a query parser and improve registration Apr 4, 2016

clintongormley added :Internal and removed :Core/Infra/Core Core issues without another label labels Apr 4, 2016

nik9000 added 2 commits April 5, 2016 14:42

Remove two query parsers and improve registration

47451a3

By default queries now register using both their snake_case and camelCase name. We no longer need to delay construction of the parser map so we can throw out QueryRegistration and no longer need Suppliers.

Move query names back to the QueryBuilders

ed98d82

nik9000 added 3 commits April 5, 2016 14:48

Move camel case handling back to queries and test alternate names

ad9c5d9

Note that this testing only works well on queries that have been cut over to the new registration API. Fixes GeohashCellQuery to match our conventions so we can find it's names.

Fixup

9dee07b

nik9000 force-pushed the prototype_stomp_query_step_2.1 branch from d485702 to 9dee07b Compare April 5, 2016 19:17

javanna reviewed Apr 6, 2016
View reviewed changes

nik9000 closed this Apr 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework a query parser and improve registration #17458

Rework a query parser and improve registration #17458

nik9000 commented Mar 31, 2016

nik9000 commented Mar 31, 2016

cbuescher Apr 1, 2016

javanna Apr 1, 2016

cbuescher Apr 1, 2016

javanna Apr 1, 2016

cbuescher commented Apr 1, 2016

nik9000 commented Apr 1, 2016

javanna Apr 1, 2016

nik9000 Apr 4, 2016

javanna commented Apr 1, 2016

cbuescher Apr 4, 2016

javanna commented Apr 5, 2016

nik9000 commented Apr 5, 2016

javanna Apr 6, 2016

javanna commented Apr 6, 2016

nik9000 commented Apr 6, 2016

Rework a query parser and improve registration #17458

Rework a query parser and improve registration #17458

Conversation

nik9000 commented Mar 31, 2016

nik9000 commented Mar 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented Apr 1, 2016

nik9000 commented Apr 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Apr 1, 2016

Choose a reason for hiding this comment

javanna commented Apr 5, 2016

nik9000 commented Apr 5, 2016

Choose a reason for hiding this comment

javanna commented Apr 6, 2016

nik9000 commented Apr 6, 2016