Added Rescore Query #441
Added Rescore Query #441
Conversation
| { | ||
| $this->setQuery($query); | ||
| $this->setParam('rescore', array()); | ||
| $this->setRescoreQuery($rescore_query); |
There was a problem hiding this comment.
Please name it $rescoreQuery based on the Elastica naming standard
|
Nice addition. Can you update the changes.txt file with your changes and fix the naming issues? |
|
Will do |
|
Looks good. I would like to see one addition. Can you also create a test where Elastica actually runs a query on elasticsearch to confirm, that also the specific elasticsearch version supports it? Like this we always know in case we upgrade to a new elasticsearch version, if the client is still compatible. Because of an other update to changes.txt (because I was too slow) there is now a conflict in changes.txt. Can you merge master and resolve it? |
|
BTW: Please add a comment when you updated it, as otherwise I don't get notified. |
|
I have added the extra test, which was a really good idea because it helped me catch an error in the Query::create method in how it handled rescore since the structure is slightly different than all other queries. I also pulled the upstream updates but they caused my latest commit to contain changes from the person who changed the filters. Is that a problem or will github handle it? |
| @@ -57,6 +57,11 @@ public static function create($query) | |||
| case $query instanceof Query: | |||
| return $query; | |||
| case $query instanceof AbstractQuery: | |||
| $query_array = $query->toArray(); | |||
There was a problem hiding this comment.
Please use the camelCase naming: $queryArray
And I assume as this is special to rescore, the "reformatting" should happen in the toArray function of Rescore and not here. Exceptions should be treated locally.
There was a problem hiding this comment.
I've been using underscores a lot recently, sorry about that.
I have already overriden the toArray method but it's only part of the solution. Every time a search happens, ultimately it will come to a Query::create. Since the Rescore extends AbstractQuery, it will currently add an extra top level of query and wrap the rescore parameters in it. So instead of
{'query':{}, 'rescore':{}} CORRECT
it will be
{'query':{'query':{}, 'rescore':{}}} INCORRECT.
The above place was the only way I saw to add support for Rescore without completely changing the way search works.
|
Somehow got an e-mail for your comment but couldn't find it anymore. One thing I just realized: Rescore is not a query type. So it should not be inside Elastica\Query*. In a way in Elastica\Search.php and Elastica\Query.php there should be a function setRescore(Elastica\Rescore) ... |
|
should that method only exist inside Elastica\Query then, since Search will create a new Query anyways and not really use setQuery itself? |
|
There is a little bit a mix between Search and Query. From my point of view, Query only exists because of legacy. In the beginning there were not facets, filters etc. so query made totally sense. I think in the long term everything should be transferred to Search. So at the moment normally it is implemented in both ... |
…earch and the accompanying tests
|
Ok, so after reading the elasticsearch api on rescore, it seems that they will probably add more types of rescores. So I have set up a new Rescore directory with an AbstractRescore that can be extended for future work. The only supported rescore currently is Query so it is there as well. I've also added setRescore in both search and query and added tests for them. |
| public function setRescore(AbstractRescore $rescore) | ||
| { | ||
| $data = $rescore->toArray(); | ||
| return $this->setParam('rescore', $data['rescore']); |
There was a problem hiding this comment.
Why do we have to use $data['rescore'] and not just $data?
There was a problem hiding this comment.
This is a side-effect of using Param as the base class of AbstractRescore
|
Somehow i miss the AbstractRescore.php file? Will there be one rescore per search or multiple? |
|
@mjhu Any updates here? I think it would be very nice to have this in the code. |
|
@ruflin Sorry for the lack of updates. After my internship ended, I kinda forgot about this. Do you like the setup of the rescorer? |
|
Yes, but I miss some files. Check my comment above (2 months ago) |
Add snapshot / restore functionality
|
@mjhu Any updates here. I would really like to have this in Elastica. |
|
Hey sorry for taking so long, but I missed your first email, and it has been slipping my mind. I also ran my tests on 1.0.1 of elasticsearch to make sure the api still works. |
|
@mjhu Thx for the update. Can you add some tests that test Rescore directly with elasticsearch (integration tests). As far as I can see the current tests all check if the models are correctly built which is good. But to detect changes and implementation issues with elasticsearch itself making calls is very helpful. |
…panded tests to test integration
|
@ruflin Good call on integration tests. Also allowed me to restructure the Rescore to follow current schemes Filters, Queries, etc. |
|
Merged, and all is green. Thx. |
|
👍 |
I have been using the new rescore feature of elasticsearch 0.9x and have had to write my own query for them. I thought this would help with others who might use the lastest api. All the parameters are in conjunction with the outline of the usage of rescore on http://www.elasticsearch.org/guide/reference/api/search/rescore/