field_value_factor accepts arrays values, does not throw error

[mattty]

In trying to figure out whether I could use multiple fields in a field_value_factor clause (which is not possible, I think?), I spotted a bug.

Passing in arrays like so:

"field_value_factor": {
  "field": ["age", "price"],
  "modifier": [ "log1p", "sqrt"],
  "factor": [1.2, 2]
}

Leads to this being executed:

"field_value_factor": {
  "field": "price",
  "modifier": "sqrt"
  "factor": 2
}

It might be better to throw an error if unexpected data types are passed in here.

As an aside, it would be great to be able to use multiple fields somehow in the field_value_factor to tune the score.

[kumartheashwani]

It seems a good one to start contributing.

[cfontes]

I tried it and I think the fix is pretty simple.

The problem is that to test this it takes a newbie like me way more effort than to fix it.

Any ideas?

[dakrone]

@cfontes you should be able to take a look at FieldValueFactorParser line 67 for where the factor is parsed and add some code to determine whether it is an array or a single value. The rest of the parsing is in this file also. The actual execution is in FieldValueFactorFunction.score.

[dakrone]

Whoops sorry, I misread your message about the testing being the hard part instead of the feature. You should be able to add a test in FunctionScoreTests that just checks that the query is not allowed and throws an exception.

[cfontes]

@dakrone thanks, I looked at that before and as I said it looks easy to fix. I ended up on those exact places.

My problem is with testing this, because as far as I can see from my limited knowledge of ES code
I would need to create valid instances of QueryParseContext parseContext, XContentParser parser which in itself is not easy task as far as I could understand ( I tryed it, not just talking the talk.)

It would be great to have a mocking framework or some easy way to get those kinds of Objects for testing purpose. Is there any already in place that I couldn't find?

[dakrone]

@cfontes you should use an integration test for this instead of mocking everything out, see this test: https://github.com/elasticsearch/elasticsearch/blob/master/src/test/java/org/elasticsearch/search/functionscore/FunctionScoreFieldValueTests.java#L41 the bottom section has some try/catches that catch invalid queries.

[cfontes]

@dakrone, thanks! I will!

Any thoughts on why that should be tested as part of an integration test? It looks like a simple case of method internal validations that looks good in a Unit test.

I don't want to be an ass or anything (sorry if it sounds like that...) just trying to understand the way you guys work.

[dakrone]

@cfontes no problem at all :)

Usually it makes sense to integration test these sort of queries because you're testing for results returned instead of an exception being thrown, so having an integration framework where you can index some documents and check query results is useful.

You can get the parseContext by using the injector to get the IndexService and unit test it if you'd like also, here's an example of getting one: https://github.com/elasticsearch/elasticsearch/blob/master/src/test/java/org/elasticsearch/index/search/FieldDataTermsFilterTests.java#L76-L80 , however, just getting the injector means starting up nodes so an integration test works just as well for this. If you do want to add a unit test for this that would be great, but not required.