Elastica\Filter\Nested should be returned back

[kukulich]

It's not possible to filter nested objects without Elastica\Filter\Nested.

Eg. Elastica\Query\Filtered::setFilter() requires \Elastica\Filter\AbstractFilter so it's not possible to use Elastica\Query\Nested.

[kukulich]

There's also an option to move all filters to Elastica\Query and change Elastica\Query\Filtered::setFilter() to accept Elastica\Query\AbstractQuery

[ruflin]

@kukulich Good point. Would it be sufficient to just make setFilter accept also AbstractQuery? What do you mean by "move all filters" to Elastica\Query? Removing filters completely?

[ruflin]

@kukulich Side note here: I think the points you are bringing up are the last major changes which are not properly done yet in Elastica 3.0.0. So I'm glad you got this discussion started.

[kukulich]

All filters are deprecated in favour of queries so this may be done:

1. Add missing queries (formerly filters), eg. ExistsQuery, MissingQuery, GeoDistanceQuery, ScriptQuery etc.
2. Remove filters completely or in favour of easier migration mark them only deprecated
3. Make setFilter() accept only AbstractQuery or in favour of easier migration make it accept both AbstractQuery and AbstractFilter.
4. Do the same for aggregations.
5. Return Elastica\Filter\Nested and mark it deprecated if other filters will be deprecated too

Side note: Yes, I tried to migrate to Elastica 3.0.0 and found these issues :)

[ruflin]

@kukulich That looks like the cleanest way to go. Mentioning @ewgRa here as he probably also has an opinion here.

A shortcut I was thinking of (in case we don't find the time to do the full change) could be to make AbstractFilter extend AbstractQuery and make setFilter() accept only AbstractQuery.

[ewgRa]

Looks interesting, but I can't define how right it will be, especially without code cases, so feel free to ignore my 2 cents.

1. https://www.elastic.co/guide/en/elasticsearch/reference/2.0/query-dsl-geo-distance-query.html - seems it is still filter. So, in Elastica call it "Query" seems not so right way. It is still query and filter.
2. I'm always for provide good BC layer.
3. It depends on "1.". "Make setFilter() accept only AbstractQuery" will be confuse developers. It will be read like "setSugarAmount($soltAmount)". Also it allow developer do many things that will have unexpected results. Like setFilter(BoolQuery->setBoost($boost)) - but there is no boost option for filter for example.

[kukulich]

I think it would be good to follow Elasticsearch naming and there are only queries now, see eg. https://www.elastic.co/guide/en/elasticsearch/reference/2.0/query-dsl-geo-distance-filter.html

[ewgRa]

@kukulich I agree with main idea follow Elasticsearch naming, lets look to documentation for now: https://www.elastic.co/guide/en/elasticsearch/reference/2.0/query-dsl-geo-distance-query.html

"Filters documents that"
"can be executed with a geo_distance filter:"
"The following are options allowed on the filter:"

It is all about filter.

Documentation just show how to perform geo distance queries, but GeoDistanceQuery it is FilteredQuery + Filter.

So, for me there is place for GeoDistanceFilter.

How it looks for you?

[kukulich]

@ewgRa So you suggest to have both GeoDistanceQuery and GeoDistanceFilter in Elastica?

[ewgRa]

@kukulich

For me it looks like GeoDistanceQuery = Query + GeoDistanceFilter.

Can you give example in code how you see it?

for me it looks like this:

$query = new Filtered();
$query->setFilter(new GeoDistanceFilter);

If for example Bool also support setFilter, I can do something like this:

$query = new BoolQuery();
$query->setFilter(new GeoDistanceFilter);

[ewgRa]

I suggest have only GeoDistanceFilter.

Have GeoDistanceQuery - it will be kind of synthetic sugar. Elasticsearch allow to filter by geodistance any queries. Would be nice to see how you see it.

[kukulich]

I understand your arguments however I think that $query->setFilter(new GeoDistanceQuery); would better follow Elasticsearch naming so it would be easier to find it in documentation.

[ruflin]

Which one has less "features"? Query or Filter? The reason I ask is to decide which one should extend which one. It seems like every filter can also be used as a query but a query could have additional features?

[ewgRa]

@kukulich Sure. Can you show code how you see it?

Something like this?

$query = new GeoDistanceQuery();
$query->setDistance...

I'm not sure (is this possible on ES side, maybe it is not a working case, haven't time to check), but how it will be handle than such ES queries?

{
    "bool": {
        "filter": {
             'and' : { 
                 ....
                 "geo_distance": ...
                 "geo_shape": ...
        }
    }
}

When query have several filters?

[ewgRa]

@ruflin As from this https://www.elastic.co/guide/en/elasticsearch/reference/current/query-filter-context.html than Query extends from Filter.

[ruflin]

@ewgRa That means we should probably also do that in our code.

I would have to check if the setCached methods we have in here are still supported: https://github.com/ruflin/Elastica/blob/master/lib/Elastica/Filter/AbstractFilter.php

[ewgRa]

@ruflin @kukulich

So, I read docs carefully.
Seems ES 2.0 now have Queries only, but also it introduce Query context and Filter context. Query can be performed in both contexts, also as a just standalone.

"The behaviour of a query clause depends on whether it is used in query context or in filter context".

As from this https://www.elastic.co/blog/better-query-execution-coming-elasticsearch-2-0 " the _cache and _cache_key options are now deprecated and will be ignored if provided".

So, for me @kukulich right, filters must be deprecated at all, instead there is must be Queries, that can be performed in Query context, or Filter context.
For me still "setFilter(Query)" looks very strange, but it is right way and follow Elasticsearch.

[ewgRa]

@kukulich +1 for what you suggest.

+2 for good BC layer

[ewgRa]

+1 for I was not right :)
-1 for I was right :)

[ewgRa]

But in another hand this "boost" that still make no sense for filter context, but can be used in query context.

[kukulich]

@ewgRa I'm glad wed we found agreement :)

[ruflin]

@ewgRa @kukulich +1 on the suggestion. Now we "only" need a PR :-)

[ruflin]

@kukulich @ewgRa Does anyone of you have a chance to work on this?

[ewgRa]

@kukulich do you want take it?

[kukulich]

I don't know if I have time to do it this year :( Maybe in January

[ewgRa]

Same for me, but maybe I can take it earlier. I think @ruflin would be glad to have it much earlier than January, so, who first will have time - just left notice here to avoid doing same work.

[ruflin]

👍

[kukulich]

👍

[kukulich]

@ruflin Could you please revert these commits 12cecd3 15c2fd1 ? After that I can try to port our application at work to current Elastica master and test it with real data. Thanks.

[ruflin]

Can you just readd the changes?

[kukulich]

@ruflin I'm sorry, I don't understand what you mean.

[ruflin]

@kukulich You asked about reverting the commits. Instead of reverting them I suggest you just take the changes that were removed and readd them. Like this no changes of existing commits have to be made and you are not blocked by me.

[kukulich]

@ruflin Thanks, I understand it now. I thought the "readd" is typo of "read" :)

[awdng]

+1 for this. I just started porting our Application to ES2.0 and ran into this issue. In our application we have a collection of filters that all get added to a BoolFilter which then gets added to a FilteredQuery. Previously in one case we were adding a NestedFilter that contained a BoolAnd Filter that included a bunch of other filters and this NestedFilter get added to the main BoolFilter. This worked perfectly before, but by removing the NestedFilter and replacing it with a NestedQuery it seems to be not possible to construct this type of nested filtering without rewriting everything.
Not sure i fully understand the "All Filters are deprecated approach" and how it will work for complex filtering though.

In the end one of our queries for ES 1.7.x which work fine looks like this:
https://gist.github.com/awdng/936993db259d34ff2f78

BTW, what is discussed here would go into Elastica 3.1 i guess as 3.0 is already in beta ?

[ruflin]

@awdng I still hope 3.0 :-)

[ruflin]

@kukulich Any updates here?

[kukulich]

Unfortunately I have no time for this now. Maybe in the end of January.

[ruflin]

@kukulich Thanks for the heads up. @ewgRa Do you have any time to work on this earlier?

I'm not sure if I should push 3.0.0 without this ...

[ewgRa]

I think I will have time on this week and make PR

[ruflin]

@ewgRa Very nice. I was thinking to do the 3.0.0 release today without the change and release 3.1.0 as soon as we have it. I think it is good to get more people on 3.0.0 in case there are some more bugs. Would that be ok for you?

[ewgRa]

Yes, for me totally ok.

[kukulich]

I think you can release 3.0.0 if you revert the commits where you have removed Filter\Nested.

[ruflin]

@kukulich I readded the files and tests, but somehow the tests fail. See https://github.com/ruflin/Elastica/pull/1020/files#diff-80be7592e890c5e74f56b6a643b4c98eR94 for example. Do you know why?

[ewgRa]

I start work on this issue.

[ewgRa]

PR for deprecated filters #1031

[kukulich]

I tested actual master and it works perfectly.

[ruflin]

@kukulich Good to hear. Thanks.

[rparsi]

@ruflin This is related to an error in the test code (master branch).
Line 202 in Query/BoolQueryTest has $boolQuery->addFilter($termFilter);
which throws an error as the BoolQuery class does not have method addFilter (inherited or otherwise).
I'm trying to construct a simple 'bool' query with 1 'match' and 1 'term' filter.
Should I follow the code in BoolFilterTest instead?

I'm trying to do this query using Elastica, but am lost:

{
  "query": {
    "bool": {
      "must": { "match": { "name": "herman" } }
    },
    "filter": {
        "term": { "deleted": false } 
      }
  }
}

[ruflin]

@rparsi I don't hope there is an error in the tests because currently the tests are green :-) Which version of Elastica are you using?

[rparsi]

@ruflin I'm using FOSElasticaBundle which is using elastic v2.1
I was reading the tests/docs for the master branch. Hence my confusion.
My apologies.

Which branch or tag is considered the latest stable version of elastica?

[ruflin]

That is 3.2.1 today: https://github.com/ruflin/Elastica/releases

[Tobion]

Closing as the problem has been sorted out and the fusion of filters and queries is complete.