Deprecate mlt, in and geo_bbox query name shortcuts #17507

javanna · 2016-04-04T13:57:35Z

If we have a deprecated query name, at the moment we don't have a way to log any deprecation warning. With this PR we use ParseField, so that we can properly deprecate query synonyms and have one single name for each query. This PR effectively deprecates in, mlt and geo_bbox in favour of their complete names terms, more_like_this and geo_bounding_box.

javanna · 2016-04-04T13:58:44Z

This does not even compile at the moment, it is just a prototype to share with @colings86 @nik9000 and @cbuescher can have a look before I go ahead with the change. I moved only the newer registration method to ParseField to minimize the work at this first stage.

javanna · 2016-04-04T14:00:03Z

core/src/test/java/org/elasticsearch/search/SearchModuleTests.java

@@ -82,6 +83,98 @@ public void testRegisterQueryParserDuplicate() {
        }
    }

+    public void testRegisteredQueries() {
+        //TODO rework test
+        /*SearchModule module = new SearchModule(Settings.EMPTY, new NamedWriteableRegistry());


I will fix this test once we settle on how things should look, but the idea is to have here a test that verifies which queries get registered, and duplicate all the query names. I think this is the only safe way to make sure that nothing gets lost.

colings86 · 2016-04-04T14:07:16Z

@javanna I left a couple of comments but I like it

cbuescher · 2016-04-04T14:13:41Z

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

+                }
+                oldValue = parseFieldsMap.putIfAbsent(name, query.parseField);
+                if (oldValue != null) {
+                    throw new IllegalArgumentException("Query parser [" + oldValue + "] already registered for name [" + name +


nit: Query parse field?

cbuescher · 2016-04-04T14:32:41Z

I also think this looks good, two minor comments, the second is also just idea maybe for future improvements, don't know if that would work currently.

javanna · 2016-04-04T18:43:32Z

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

@@ -63,6 +63,10 @@
    /** The default name for the match query */
    public static final String NAME = "match";

+    //TODO all these alternate names are now deprecated but the MatchQueryBuilder code needs to be updated.


this is for #17508 which I think should go in first. otherwise we deprecate before having an alternative.

javanna · 2016-04-04T18:48:30Z

I updated the PR and removed the WIP label. I addressed all comments, don't get fooled by the size, it's the same small change all over the place, plus some line length problems addressed while I was at it. Most of the change is in SearchModule, where I made some of the changes that Nik made as part of #17458. We also have a test for registered queries in SearchModuleTests, let me know what you think!

nik9000 · 2016-04-04T19:16:55Z

docs/reference/query-dsl/mlt-query.asciidoc

@@ -6,7 +6,7 @@ set of documents. In order to do so, MLT selects a set of representative terms
 of these input documents, forms a query using these terms, executes the query
 and returns the results. The user controls the input documents, how the terms
 should be selected and how the query is formed. `more_like_this` can be
-shortened to `mlt`.
+shortened to `mlt` (deprecated, use `more_like_this` rather than `mlt`).


Can this have a version attached?

yea I'll have to figure out how to do that, I forgot

nik9000 · 2016-04-04T19:18:25Z

I'm fine with the code. We'll need to add stuff to the breaking changes docs I think. As well as put the versions when things were deprecated in the docs.

cbuescher · 2016-04-04T19:19:01Z

I took a look, this looks good to me.

javanna · 2016-04-04T19:56:07Z

We'll need to add stuff to the breaking changes docs I think.

but nothing is breaking here :) I am not sure whether we have a deprecation list somewhere, I will ask around.

nik9000 · 2016-04-04T20:13:00Z

The breaking changes list is the deprecated list, iirc.

colings86 · 2016-04-05T08:27:00Z

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

+    }
+
+    private <QB extends QueryBuilder<QB>> void innerRegisterQueryParser(QueryParser<QB> parser, ParseField queryName) {
+        Tuple<ParseField, QueryParser<?>> parseFieldQueryParserTuple = new Tuple<>(queryName, parser);


To me, this logic should really be in IndicesQueriesRegistry so we construct the registry with just the Settings object and then call a registerQuery(ParseField, QueryParser<?>) method which unpacks the ParseField and adds it to the registry map. That was the registry is dealing with how to internally structure the data and the internals can be easily changed later without affecting outside code.

I get it I get it :) I think it is easy to move this method to the registry at this point, since we have a single step only

I changed my mind, I think the register methods should be only here in SearchModule. The nice thing of IndicesQueriesRegistry is that it's immutable, and classes that don't depend on it do so only because they need to read from it, retrieve registered parsers. I wouldn't want to expose a register method there and make the class mutable. You see what I mean?

Hmmm, I do see what you mean. Personally I would still prefer the register method in the registry but I think this is a personal preference thing rather than a substantial concern so I'm happy to yield on it 😄

colings86 · 2016-04-05T08:56:44Z

@javanna I added one more comment but otherwise it LGTM

colings86 · 2016-04-05T13:05:45Z

LGTM

If we have a deprecated query name, at the moment we don't have a way to log any deprecation warning nor fail when we are in strict mode. With this change we use ParseField, which will take care of the camel casing that we currently do manually (so that one day we can remove it more easily). This also means, that each query will have a unique preferred name, and all the other names are deprecated. Terms query "in" synonym is now formally deprecated, as well as fuzzy_match, match_fuzzy, match_phrase and match_phrase_prefix for match query, mlt for more_like_this and geo_bbox for geo_bounding_box. All these will be removed in 6.0. Every QueryParser holds now a ParseField constant called QUERY_NAME_FIELD that holds the name for it. The first name is the preferred one, all the others are deprecated. The first name is taken from the NAME constant already present in each query builder object, so that we somehow keep the serialization constant separated from ParseField. This change also allowed us to remove the names method from the QueryParser interface.

See elastic/elasticsearch#17507

javanna added >enhancement WIP :Query DSL v5.0.0-alpha2 labels Apr 4, 2016

javanna reviewed Apr 4, 2016
View reviewed changes

cbuescher reviewed Apr 4, 2016
View reviewed changes

javanna force-pushed the enhancement/log_deprecation_query_names branch 2 times, most recently from 8ed6319 to 221e8dd Compare April 4, 2016 18:42

javanna reviewed Apr 4, 2016
View reviewed changes

javanna added review and removed WIP labels Apr 4, 2016

nik9000 reviewed Apr 4, 2016
View reviewed changes

javanna added the >deprecation label Apr 4, 2016

colings86 reviewed Apr 5, 2016
View reviewed changes

[TEST] check registered queries one by one in SearchModuleTests

c80351b

javanna force-pushed the enhancement/log_deprecation_query_names branch 2 times, most recently from 8c2e269 to 6c4128b Compare April 5, 2016 12:56

javanna force-pushed the enhancement/log_deprecation_query_names branch from 6c4128b to dae9987 Compare April 5, 2016 13:15

javanna merged commit 67ccfc3 into elastic:master Apr 5, 2016

nik9000 mentioned this pull request Apr 5, 2016

Rework a query parser and improve registration #17458

Closed

clintongormley removed the >enhancement label Apr 6, 2016

javanna changed the title ~~Switch to using ParseField to parse query names~~ Deprecate mlt, in and geo_bbox query name shortcuts May 2, 2016

russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 14, 2016

Deprecate mlt short name in favour of more_like_this

b8bdc23

See elastic/elasticsearch#17507

russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 16, 2016

Deprecate mlt short name in favour of more_like_this

d4211ae

See elastic/elasticsearch#17507

This was referenced Apr 6, 2017

swap deprecated geo_bbox to geo_bounding_box olivere/elastic#506

Closed

mlt was deprecated for more_like_this olivere/elastic#507

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate mlt, in and geo_bbox query name shortcuts #17507

Deprecate mlt, in and geo_bbox query name shortcuts #17507

javanna commented Apr 4, 2016 •

edited

Loading

javanna commented Apr 4, 2016

javanna Apr 4, 2016

colings86 commented Apr 4, 2016

cbuescher Apr 4, 2016

cbuescher commented Apr 4, 2016

javanna Apr 4, 2016

javanna commented Apr 4, 2016

nik9000 Apr 4, 2016

javanna Apr 4, 2016

nik9000 commented Apr 4, 2016

cbuescher commented Apr 4, 2016

javanna commented Apr 4, 2016

nik9000 commented Apr 4, 2016

colings86 Apr 5, 2016

javanna Apr 5, 2016

javanna Apr 5, 2016

colings86 Apr 5, 2016

colings86 commented Apr 5, 2016

colings86 commented Apr 5, 2016

Deprecate mlt, in and geo_bbox query name shortcuts #17507

Deprecate mlt, in and geo_bbox query name shortcuts #17507

Conversation

javanna commented Apr 4, 2016 • edited Loading

javanna commented Apr 4, 2016

Choose a reason for hiding this comment

colings86 commented Apr 4, 2016

Choose a reason for hiding this comment

cbuescher commented Apr 4, 2016

Choose a reason for hiding this comment

javanna commented Apr 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Apr 4, 2016

cbuescher commented Apr 4, 2016

javanna commented Apr 4, 2016

nik9000 commented Apr 4, 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

colings86 commented Apr 5, 2016

colings86 commented Apr 5, 2016

javanna commented Apr 4, 2016 •

edited

Loading