Geodistance filter, missing options

[pierrre]

There are missing options:

• distance_type
• optimize_bbox

Do you have any recommendation?
Are class constants required? (arc, plane, memory, indexed, none)

[pierrre]

Hmmm, I think we should completely rewrite Elastica_Filter_GeoDistance.
Setters don't use setParam() (they use protected variables).
toArray() builds the array manually, so "_cache" option is not usable...

[pierrre]

It should also support location as geohash:
http://www.elasticsearch.org/guide/reference/query-dsl/geo-distance-filter.html

There is a backward compatibility break in the constructor (PHP is a stupid language).

[ruflin]

Missing options: I think here we can use some constants.

Concerning setParam(): Yes, we should to that. I already did it for most of the queries and filters, but not all.

Backward compatitbility: Question is, what has first priority. Probably all params in the constructor should be optimal. What would be your suggestion?

PHP: Every language has advantages and disadvantages ;-)

[pierrre]

All currently used parameters are mandatory:

• key (field name)
• location (latitude+longitude OR geohash)
• distance (string format)

Other parameters are optional:

• distance_type
• optimize bbox

The problem is: in the current constructor, only latitude+longitude is supported, not geohash.
And we can't change that without BC break.

[ruflin]

I agree on that, but what would be the solution to allow both? Because of PHP, we cannot have two different constructors except in the case that we use two different class which inherit from an abstract class ...

BTW: I'm currently on holiday, so I'm probably going to respond less frequently ;-)

[pierrre]

We could use func_get_args(), but it's really shitty...
Maybe we should break the backward compatibility for this constructor...
It is also possible to implement missing options now, and implement geohash later :P

Have a great holiday!
(I'd like to fix #76 before implementing other features)

[ruflin]

Ugh, I don't like the func_get_args function at all. I agree on breaking BC, but I don't know yet how the new implementation would work? Example?

[pierrre]

public function __construct($key, $location, $distance)

$location can be:

• an array array(48.86, 2.35) or an associative array array('latitude' => 48.86, 'longitude' => 2.35)
• a string for geohash

[ruflin]

I quite like this solution. We could add a "deprecated" for the next to releases where we check if there are 4 params and then would convert the old params to the new version bit a function that is deprecated. Like this, people that use the never version should realize.

Please make sure to always update changes.txt with your changes because I use this file for the release notes and it makes it easy for people to look up relevant changes.

[pierrre]

Hmmm, there would be 2 constructors :P
It's not possible in PHP, right?

[ruflin]

Hm, somehow my e-mail answer did not make it into github.

There would be only 1 constructor, be we would use the ugly func_get_args function to check for a fourth param. If we have a fourth param, we call a function (which is deprecated) to remodel the arguments to the new structure. This code will only exist for 1-2 releases, after that we are going to remove it.

[ruflin]

no, there would be an if clause inside the constructor

On 26.06.2012, at 00:28, Pierre Durand reply@reply.github.com wrote:

Hmmm, there would be 2 constructors :P
It's not possible in PHP, right?

---

Reply to this email directly or view it on GitHub:
#200 (comment)

[pierrre]

It could call the constructor again with 3 arguments :P

[ruflin]

right. even better

[pierrre]

We could also use the "distance_unit" parameters, instead of the (dirty) current format.
http://www.elasticsearch.org/guide/reference/query-dsl/geo-distance-filter.html

Currently, the "distance" parameter is a string ("19km").

It is possible to use the "distance" parameters as a number (19), and add the "distance_unit" parameters ("km").
I think it is better.

It will be easier to validate

    public function setDistance($distance) {
        // TODO: validate distance?
        $this->_distance = $distance;
        return $this;
    }

[pierrre]

Is it a good idea to have setDistance() / setLatitude() / setLongitude() methods, because these parameters are mandatory in the constructor?

[pierrre]

I just cleaned the geodistance filter:
954f1b2

Is it ok?

[ruflin]

Finally, my feedback:

• Is is possible to use both, raw distance and distance_unit, so the user can choose?
• I think it is good to have the setters available, as it should also possible to modify the values after the object has been instantiated
• See comments int he pull request ;-)

[pierrre]

It is really difficult to implement raw distance + distance unit & distance, because we have to support the old constructor (for retrocompatibility).
If we drop the old constructor, it could be easier to implement distance unit.

It is not easy to have setters for latitude, longitude, or geohash, because they require the "key" parameter.
If we create the filter with the geohash parameter, what happen if we call setLatitude() alone????

Maybe we could add a setLocation($key, $location) method.
It also means we'll have to unset the previous value in the "params".

What should we do?

[ruflin]

Ah, this strange thing named BC again ;-)

A suggestion from my side: We keep the old constructor, but split up the distance and the unit (regexp, ...). We then store it seperate in the object, which also allows the setter.

Fro the setLatitude(): I would argue, nothing happens until toArray() is called, where an exception is thrown. Either the geohash or the longitude AND latitude have to be set.

I would suggest, that we have a seperate function setKey(...).

Perhaps things get simpler, if we do not store latitude / longitude already in an array (even though it is passed in an array) and create the structure for the request in toArray().

Somehow it doesn't feel 100% logical yet, also what I described above :-(

[pierrre]

Do you think we need distance + unit feature?

[ruflin]

I would be nice ;-)

[pierrre]

I have an idea: the last parameter could be a string (distance + unit as string) or an associative array (distance as float, and unit as string)

We could also add a setter: setDistance($distance, $unit = null)
Unit is optionnal.

[ruflin]

I like both :-) And if only a string is passed to setDistance? Will the function try to split it up?

[pierrre]

No, it will use the parameter as a string.
If unit is null, we cast the first argument as a string, and use the basic filtre structure.
If distance is not null, we cast the first argument as a float and the second as a string, and we use the advanced filter structure.

[ruflin]

Right, forgot that there were two structures. Sorry.

[ruflin]

Any updates here?

[ruflin]

@pierrre As you closed this, I assume this is not needed anymore or already implemented.

[pierrre]

Yes, I closed all my old issues: https://github.com/pierrre?tab=activity
I use the official PHP client.

[ruflin]

Ok, thx. Even though I would prefer that you would still use Elastica ;-)

[pierrre]

:-(