ruflin / Elastica

Elastica is a PHP client for elasticsearch
http://elastica.io/
MIT License
2.26k stars 736 forks source link

Percolation issue #896

Open im-denisenko opened 9 years ago

im-denisenko commented 9 years ago

Current Percolator implementation has a few problems:

My suggestions:

I would like to propose PR with these changes. Any thoughts?

ruflin commented 9 years ago

An alternative suggestion. Percolate has two steps:

It could make sense to separate the two. The query is stored as document on top of an index. So it makes sense to haven index for the "Percolator" object. The percolate function itself is the same as a query or search.

For me the Search object should be the single point of access for all "searchs" in the future. As Percolate is very similar, perhaps we should add it to the Search object instead of creating a new one.

So what we would end up with would be:

im-denisenko commented 9 years ago

Keep Percolator object but only to the registration of a query part

Agreed.

Move percolate queries to the Search objects.

But why? Request to _search endpoint and request to _percolate endpoint may be similar in some particular points, but they differs so much, i don't sure they could be merged into one class. Maximum that we can do here is to move part of Elastica\Search class into traits and reuse them in Elastica\Percolate, am I miss something?

ruflin commented 9 years ago

The two functions we are talking about are matchDoc and matchExistingDoc. The use a completely different path but don't have any "real" complexity. I see it more from a conceptual point where I would look for these too functions. I would probably search for matchDoc in the Search object.

Probably the better option is to go with the Percolate object you suggested above and then either use a proxy inside search or go with traits. As so far with most of the time used proxy methods, I would suggest to use this.