added geotile grid support

[krewetka]

In ElasticSearch 7 and above there is new aggregation type which is not supported here yet -> GeoTile Grid

I have added support of it. It is very similar to existing GeoHash Grid except it has more detailed precision levels.

I also renamed one test in other class which had incorrect name.

[ruflin]

Hi, this looks good. Could you add a changelog?

There is an other issue we have at the moment with travis and CI is not run. I would like to wait with getting this PR in until we have CI running again if that is ok for you?

[krewetka]

Hi, this looks good. Could you add a changelog?

sure I will add it there :)

I would like to wait with getting this PR in until we have CI running again if that is ok for you?

if this will be a week or so sure, no problem. I have switched my project to use fork right now

[ruflin]

@krewetka As soon as #1885 is merged, could you rebase your PR?

[deguif]

It's merged

[deguif]

We should probably start marking these kind of class as final as @thePanz recently commented.
And as it's a new class also suffix its name with Aggregation. This requires #1887 to be merged before.

[krewetka]

ok, no problem

[deguif]

ping @ruflin for his confirmation.

[krewetka]

@ruflin is this ok for you? prefer confirming before doing this changes

[ruflin]

@deguif can you point me to the final comment? I'm not sure I fully follow why we should make it final? Should we really prevent others from extending these classes?

[deguif]

Let's not mark the class as final for now as it would need discussing this subject more in details.
Will the name GeotileGridAggregation be OK for you?

[ruflin]

@deguif Currently we always skip the Aggregation part as it is already in the name of the directory. So to be consistent it should be GoetileGrid?

[deguif]

Yes but as stated in #1824 I would be in favor to deprecate all aggregations classes and suffix them with Aggregation to avoid them colliding with reserved keywords and unifying all classes names.

Here's the class is new so it could directly be named accordingly and avoid having to deprecate it later.
Would that sound good for you?

[krewetka]

@ruflin @deguif please align about it. I really would like it to be merged soon - one way or another ;)

I have added this Aggregation at the end per request here. I have also changed code in my project to support this new name already.

Btw, I personally like adding Aggregation to the name. Easlier my projects code looked like this

use Elastica\Aggregation\GeotileGrid as GeotileGridAggregation;

for readibility reason later in code. Otherwise it is much less visible in code where it is used that this is aggregation.

[deguif]

Thanks @krewetka for your PR.

I see on the elasticsearch documentation that there is also an optional bounds parameter, it can probably be added without too much work. WDYT?

For details: https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-geotilegrid-aggregation.html#_requests_with_additional_bounding_box_filtering_2

[krewetka]

I see on the elasticsearch documentation that there is also an optional bounds parameter, it can probably be added without too much work. WDYT?

Yes, it could. But it also exists in geohash grid and is not there in Ruflin elastica so maybe better as separate PR adding it to both? 🤔

[ruflin]

@krewetka Getting an additional PR later on SGTM.

[krewetka]

@krewetka Getting an additional PR later on SGTM.

ok :) so this one should be ready IMO to be merged @ruflin , @deguif

[krewetka]

Thanks @krewetka for your PR.

I see on the elasticsearch documentation that there is also an optional bounds parameter, it can probably be added without too much work. WDYT?

For details: https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-geotilegrid-aggregation.html#_requests_with_additional_bounding_box_filtering_2

@deguif after checking this parameter and info that is supports alll formats from https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-geo-bounding-box-query.html#query-dsl-geo-bounding-box-query-accepted-formats I am not sure I am able to handle this one propely.

I have checked https://github.com/ruflin/Elastica/blob/a2f5b0d7171691da06a4c31b6829b9c1dafdfbf5/src/Query/GeoBoundingBox.php and even for original one there is just one format supported now in ruflin/Eastica

I can try to add the same one type for both Geohash and Geotile but to be honest I don't have time for more, not the knowledge how to do it properly :)

[deguif]

@krewetka no problem for that, that's not a blocker for me.
Thank you for this nice contribution and as @ruflin already commented we can add this parameter later ;)

[ruflin]

LGTM. Did a commit to resolve the changelog conflict. @deguif If ok for you, please merge directly.

[krewetka]

Thanks for merging :)