Function Score: Add optional weight parameter per function #7137

brwe · 2014-08-03T16:07:45Z

Weights can be defined per function like this:

"function_score": {
    "functions": [
        {
            "filter": {},
            "FUNCTION": {},
            "weight": number
        }
        ...

If weight is given without FUNCTION then weight behaves like boost_factor.
This commit deprecates boost_factor.

The following is valid:

POST testidx/_search
{
  "query": {
    "function_score": {
      "weight": 2
    }
  }
}
POST testidx/_search
{
  "query": {
    "function_score": {
      "functions": [
        {
          "weight": 2
        },
        ...
      ]
    }
  }
}
POST testidx/_search
{
  "query": {
    "function_score": {
      "functions": [
        {
          "FUNCTION": {},
          "weight": 2
        },
        ...
      ]
    }
  }
}
POST testidx/_search
{
  "query": {
    "function_score": {
      "functions": [
        {
          "filter": {},
          "weight": 2
        },
        ...
      ]
    }
  }
}
POST testidx/_search
{
  "query": {
    "function_score": {
      "functions": [
        {
          "filter": {},
          "FUNCTION": {},
          "weight": 2
        },
        ...
      ]
    }
  }
}

The following is not valid:

POST testidx/_search
{
  "query": {
    "function_score": {
      "weight": 2,
      "FUNCTION(including boost_factor)": 2
    }
  }
}

POST testidx/_search
{
  "query": {
    "function_score": {
      "functions": [
        {
          "weight": 2,
          "boost_factor": 2
        }
      ]
    }
  }
}

closes #6955

clintongormley · 2014-08-04T06:28:07Z

++ to deprecating boost_factor in favour of weight. a bold move, but the right thing to do

jpountz · 2014-08-04T06:42:00Z

docs/reference/query-dsl/queries/function-score-query.asciidoc

@@ -69,6 +74,9 @@ First, each document is scored by the defined functions. The parameter
 `max`::         maximum score is used
 `min`::         minimum score is used

+Because scores can be on different scales (for example, between 0 and 1 for decay functions but arbitrary for `random_score`), the score of each function can be adjusted with a user defined `weight` (added[1.4.0]). The `weight` can be defined per function in the `functions` array (example above) and is multiplied with the score computed by the respective function.


The arbitrary scale for random_score might be fixed soon so maybe we should rather give another example?

jpountz · 2014-08-04T06:56:16Z

You added the weight as a common property of all functions, but I'm wondering if that wouldn't make more sense to keep that out of ScoreFunction and to apply the weight on top of the function. For example, maybe we could have a WeightFunction that would wrap a weight (double) and another function, and when a weight that is != 1 would be provided, we would wrap the function in a WeightFunction instance?

brwe · 2014-08-11T15:05:56Z

Thanks for the review! Implemented all suggestions.

jpountz · 2014-08-12T06:36:47Z

src/main/java/org/elasticsearch/common/lucene/search/function/WeightFactorFunction.java

+
+    @Override
+    public String toString() {
+        return "weight[" + getWeight() + "]";


Should it mention the wrapped function as well?

The explanation contains the wrapped function already, example below. Is this what you meant?
In the example the string describing the decay function misses a [, will fix in a different commit.

{ "explain": true, "query": { "function_score": { "query": { "match": { "text": "foo" } }, "functions": [ { "linear": { "number": { "origin": "1", "scale": "5" } }, "weight": 10 } ] } } }

result:

"_explanation": { "value": 1.726047, "description": "function score, product of:", "details": [ { "value": 0.19178301, "description": "weight(text:foo in 0) [PerFieldSimilarity], result of:", "details": [ { "value": 0.19178301, "description": "fieldWeight in 0, product of:", "details": [ { "value": 1, "description": "tf(freq=1.0), with freq of:", "details": [ { "value": 1, "description": "termFreq=1.0" } ] }, { "value": 0.30685282, "description": "idf(docFreq=1, maxDocs=1)" }, { "value": 0.625, "description": "fieldNorm(doc=0)" } ] } ] }, { "value": 9, "description": "Math.min of", "details": [ { "value": 9, "description": "product of:", "details": [ { "value": 0.9, "description": "Function for field number:", "details": [ { "value": 0.9, "description": "max(0.0, ((10.0 - MINMath.max(Math.abs(2.0(=doc value) - 1.0(=origin))) - 0.0(=offset), 0)])/10.0)" } ] }, { "value": 10, "description": "weight" } ] }, { "value": 3.4028235e+38, "description": "maxBoost" } ] }, { "value": 1, "description": "queryBoost" } ] }

Forget it, has nothing to do with the explanation at all...

So, this is an artifact left over from copying BoostScoreFunction.The method is actually not needed at all. I will remove it.

jpountz · 2014-08-12T06:58:43Z

@brwe Left some more comments

brwe · 2014-08-13T12:56:45Z

Implemented all. However, while I took a closer look at the score explanation I found several issues #7257 #7248 #7245 . I would like to wait until these are merged because then I can write a proper test for explain. The current one is ugly: https://github.com/elasticsearch/elasticsearch/pull/7137/files#diff-f10a23353b65e87c30eb7c037e718f1fR97

brwe · 2014-08-27T15:56:17Z

Rebased on master and fixed the explain test now. Ready for the next review.

rjernst · 2014-08-27T16:04:48Z

docs/reference/query-dsl/queries/function-score-query.asciidoc

@@ -9,7 +9,7 @@ the score on a filtered set of documents.
 `function_score` provides the same functionality that
 `custom_boost_factor`, `custom_score` and
 `custom_filters_score` provided
-but furthermore adds futher scoring functionality such as
+but furthermore adds further scoring functionality such as


This was odd wording to begin with. Can we change to:

"but with additional capabilities such as"

yes, that's better

brwe · 2014-08-29T08:34:37Z

Thanks for the review! Addressed all comments and ready for round four.

… even if disabled Settings that are not default for _size, _index and _timestamp were only build in toXContent if these fields were actually enabled. _timestamp, _index and _size can be dynamically enabled or disabled. Therfore the settings must be kept, even if the field is disabled. (Dynamic enabling/disabling was intended, see TimestampFieldMapper.merge(..) and SizeMappingTests#testThatDisablingWorksWhenMerging but actually never worked, see below). To avoid that _timestamp is overwritten by a default mapping this commit also adds a check to mapping merging if the type is already in the mapping. In this case the default is not applied anymore. (see SimpleTimestampTests#testThatUpdatingMappingShouldNotRemoveTimestampConfiguration) As a side effect, this fixes - overwriting of paramters from the _source field by default mappings (see DefaultSourceMappingTests). - dynamic enabling and disabling of _timestamp and _size () (see SimpleTimestampTests#testThatTimestampCanBeSwitchedOnAndOff and SizeMappingIntegrationTests#testThatTimestampCanBeSwitchedOnAndOff ) Tests: Enable UpdateMappingOnClusterTests#test_doc_valuesInvalidMappingOnUpdate again The missing settings in the mapping for _timestamp, _index and _size caused a the failure: When creating a mapping which has settings other than default and the field disabled, still empty field mappings were built from the type mappers. When creating such a mapping, the mapping source on master and the rest of the cluster can be out of sync for some time: 1. Master creates the index with source _timestamp:{_store:true} mapper classes are in a correct state but source is _timestamp:{} 2. Nodes update mapping and refresh source which then completely misses _timestamp 3. After a while source is refreshed again also on master and the _timestamp:{} vanishes there also. The test UpdateMappingOnCusterTests#test_doc_valuesInvalidMappingOnUpdate failed because the cluster state was sampled from master between 1. and 3. because the randomized testing injected a default mapping with disabled _size and _timestamp fields that have settings which are not default. The test TimestampMappingTests#testThatDisablingFieldMapperDoesNotReturnAnyUselessInfo must be removed because it actualy expected the timestamp to remove parameters when it was disabled. closes elastic#7137

jpountz · 2014-08-29T12:58:44Z

src/main/java/org/elasticsearch/index/query/functionscore/weight/WeightBuilder.java

+
+    @Override
+    public String getName() {
+        throw new UnsupportedOperationException("weight factor is never supposed to be parsed");


Since this is part of the client API, I think it needs to be implemented?

jpountz · 2014-08-29T12:59:25Z

Just left one minor comment, other than that LGTM

… even if disabled Settings that are not default for _size, _index and _timestamp were only build in toXContent if these fields were actually enabled. _timestamp, _index and _size can be dynamically enabled or disabled. Therfore the settings must be kept, even if the field is disabled. (Dynamic enabling/disabling was intended, see TimestampFieldMapper.merge(..) and SizeMappingTests#testThatDisablingWorksWhenMerging but actually never worked, see below). To avoid that _timestamp is overwritten by a default mapping this commit also adds a check to mapping merging if the type is already in the mapping. In this case the default is not applied anymore. (see SimpleTimestampTests#testThatUpdatingMappingShouldNotRemoveTimestampConfiguration) As a side effect, this fixes - overwriting of paramters from the _source field by default mappings (see DefaultSourceMappingTests). - dynamic enabling and disabling of _timestamp and _size () (see SimpleTimestampTests#testThatTimestampCanBeSwitchedOnAndOff and SizeMappingIntegrationTests#testThatTimestampCanBeSwitchedOnAndOff ) Tests: Enable UpdateMappingOnClusterTests#test_doc_valuesInvalidMappingOnUpdate again The missing settings in the mapping for _timestamp, _index and _size caused a the failure: When creating a mapping which has settings other than default and the field disabled, still empty field mappings were built from the type mappers. When creating such a mapping, the mapping source on master and the rest of the cluster can be out of sync for some time: 1. Master creates the index with source _timestamp:{_store:true} mapper classes are in a correct state but source is _timestamp:{} 2. Nodes update mapping and refresh source which then completely misses _timestamp 3. After a while source is refreshed again also on master and the _timestamp:{} vanishes there also. The test UpdateMappingOnCusterTests#test_doc_valuesInvalidMappingOnUpdate failed because the cluster state was sampled from master between 1. and 3. because the randomized testing injected a default mapping with disabled _size and _timestamp fields that have settings which are not default. The test TimestampMappingTests#testThatDisablingFieldMapperDoesNotReturnAnyUselessInfo must be removed because it actualy expected the timestamp to remove parameters when it was disabled. closes #7137

brwe · 2014-09-01T08:42:42Z

autsch...wrong issue number in commit message for 9750375

Weights can be defined per function like this: ``` "function_score": { "functions": [ { "filter": {}, "FUNCTION": {}, "weight": number } ... ``` If `weight` is given without `FUNCTION` then `weight` behaves like `boost_factor`. This commit deprecates `boost_factor`. The following is valid: ``` POST testidx/_search { "query": { "function_score": { "weight": 2 } } } POST testidx/_search { "query": { "function_score": { "functions": [ { "weight": 2 }, ... ] } } } POST testidx/_search { "query": { "function_score": { "functions": [ { "FUNCTION": {}, "weight": 2 }, ... ] } } } POST testidx/_search { "query": { "function_score": { "functions": [ { "filter": {}, "weight": 2 }, ... ] } } } POST testidx/_search { "query": { "function_score": { "functions": [ { "filter": {}, "FUNCTION": {}, "weight": 2 }, ... ] } } } ``` The following is not valid: ``` POST testidx/_search { "query": { "function_score": { "weight": 2, "FUNCTION(including boost_factor)": 2 } } } POST testidx/_search { "query": { "function_score": { "functions": [ { "weight": 2, "boost_factor": 2 } ] } } } ```` closes #6955 closes #7137

… even if disabled Settings that are not default for _size, _index and _timestamp were only build in toXContent if these fields were actually enabled. _timestamp, _index and _size can be dynamically enabled or disabled. Therfore the settings must be kept, even if the field is disabled. (Dynamic enabling/disabling was intended, see TimestampFieldMapper.merge(..) and SizeMappingTests#testThatDisablingWorksWhenMerging but actually never worked, see below). To avoid that _timestamp is overwritten by a default mapping this commit also adds a check to mapping merging if the type is already in the mapping. In this case the default is not applied anymore. (see SimpleTimestampTests#testThatUpdatingMappingShouldNotRemoveTimestampConfiguration) As a side effect, this fixes - overwriting of paramters from the _source field by default mappings (see DefaultSourceMappingTests). - dynamic enabling and disabling of _timestamp and _size () (see SimpleTimestampTests#testThatTimestampCanBeSwitchedOnAndOff and SizeMappingIntegrationTests#testThatTimestampCanBeSwitchedOnAndOff ) Tests: Enable UpdateMappingOnClusterTests#test_doc_valuesInvalidMappingOnUpdate again The missing settings in the mapping for _timestamp, _index and _size caused a the failure: When creating a mapping which has settings other than default and the field disabled, still empty field mappings were built from the type mappers. When creating such a mapping, the mapping source on master and the rest of the cluster can be out of sync for some time: 1. Master creates the index with source _timestamp:{_store:true} mapper classes are in a correct state but source is _timestamp:{} 2. Nodes update mapping and refresh source which then completely misses _timestamp 3. After a while source is refreshed again also on master and the _timestamp:{} vanishes there also. The test UpdateMappingOnCusterTests#test_doc_valuesInvalidMappingOnUpdate failed because the cluster state was sampled from master between 1. and 3. because the randomized testing injected a default mapping with disabled _size and _timestamp fields that have settings which are not default. The test TimestampMappingTests#testThatDisablingFieldMapperDoesNotReturnAnyUselessInfo must be removed because it actualy expected the timestamp to remove parameters when it was disabled. closes #7137

Weights can be defined per function like this: ``` "function_score": { "functions": [ { "filter": {}, "FUNCTION": {}, "weight": number } ... ``` If `weight` is given without `FUNCTION` then `weight` behaves like `boost_factor`. This commit deprecates `boost_factor`. The following is valid: ``` POST testidx/_search { "query": { "function_score": { "weight": 2 } } } POST testidx/_search { "query": { "function_score": { "functions": [ { "weight": 2 }, ... ] } } } POST testidx/_search { "query": { "function_score": { "functions": [ { "FUNCTION": {}, "weight": 2 }, ... ] } } } POST testidx/_search { "query": { "function_score": { "functions": [ { "filter": {}, "weight": 2 }, ... ] } } } POST testidx/_search { "query": { "function_score": { "functions": [ { "filter": {}, "FUNCTION": {}, "weight": 2 }, ... ] } } } ``` The following is not valid: ``` POST testidx/_search { "query": { "function_score": { "weight": 2, "FUNCTION(including boost_factor)": 2 } } } POST testidx/_search { "query": { "function_score": { "functions": [ { "weight": 2, "boost_factor": 2 } ] } } } ```` closes #6955 closes #7137

… even if disabled Settings that are not default for _size, _index and _timestamp were only build in toXContent if these fields were actually enabled. _timestamp, _index and _size can be dynamically enabled or disabled. Therfore the settings must be kept, even if the field is disabled. (Dynamic enabling/disabling was intended, see TimestampFieldMapper.merge(..) and SizeMappingTests#testThatDisablingWorksWhenMerging but actually never worked, see below). To avoid that _timestamp is overwritten by a default mapping this commit also adds a check to mapping merging if the type is already in the mapping. In this case the default is not applied anymore. (see SimpleTimestampTests#testThatUpdatingMappingShouldNotRemoveTimestampConfiguration) As a side effect, this fixes - overwriting of paramters from the _source field by default mappings (see DefaultSourceMappingTests). - dynamic enabling and disabling of _timestamp and _size () (see SimpleTimestampTests#testThatTimestampCanBeSwitchedOnAndOff and SizeMappingIntegrationTests#testThatTimestampCanBeSwitchedOnAndOff ) Tests: Enable UpdateMappingOnClusterTests#test_doc_valuesInvalidMappingOnUpdate again The missing settings in the mapping for _timestamp, _index and _size caused a the failure: When creating a mapping which has settings other than default and the field disabled, still empty field mappings were built from the type mappers. When creating such a mapping, the mapping source on master and the rest of the cluster can be out of sync for some time: 1. Master creates the index with source _timestamp:{_store:true} mapper classes are in a correct state but source is _timestamp:{} 2. Nodes update mapping and refresh source which then completely misses _timestamp 3. After a while source is refreshed again also on master and the _timestamp:{} vanishes there also. The test UpdateMappingOnCusterTests#test_doc_valuesInvalidMappingOnUpdate failed because the cluster state was sampled from master between 1. and 3. because the randomized testing injected a default mapping with disabled _size and _timestamp fields that have settings which are not default. The test TimestampMappingTests#testThatDisablingFieldMapperDoesNotReturnAnyUselessInfo must be removed because it actualy expected the timestamp to remove parameters when it was disabled. closes elastic#7137

brwe added the review label Aug 3, 2014

jpountz reviewed Aug 4, 2014
View reviewed changes

jpountz removed the review label Aug 4, 2014

clintongormley assigned brwe Aug 7, 2014

brwe added the review label Aug 11, 2014

jpountz reviewed Aug 12, 2014
View reviewed changes

jpountz removed the review label Aug 12, 2014

clintongormley added v1.4.0 labels Aug 20, 2014

brwe removed their assignment Aug 21, 2014

clintongormley assigned brwe Aug 22, 2014

brwe force-pushed the issue-6955 branch 2 times, most recently from d2b19c8 to 30b0c17 Compare August 27, 2014 15:50

brwe added the review label Aug 27, 2014

rjernst reviewed Aug 27, 2014
View reviewed changes

brwe added 4 commits August 29, 2014 10:31

double -> float

73e380e

weigh of 1.0 still produces weight function

92098b3

prefix check -> test

467a682

inline builders and add error message to fail()

f006bdd

brwe force-pushed the issue-6955 branch from 30b0c17 to f006bdd Compare August 29, 2014 08:31

jpountz reviewed Aug 29, 2014
View reviewed changes

jpountz removed the review label Aug 29, 2014

brwe closed this in 9750375 Sep 1, 2014

clintongormley added v2.0.0 labels Sep 1, 2014

brwe reopened this Sep 1, 2014

do not throw UnsupportedOperationException

6373054

brwe closed this in c5ff70b Sep 1, 2014

clintongormley changed the title ~~function_score: add optional weight parameter per function~~ Function Score: Add optional weight parameter per function Sep 8, 2014

ghiron mentioned this pull request Dec 17, 2014

Function score and optional weight : avg score is wrong #8992

Closed

clintongormley added the :Query DSL label Jun 7, 2015

clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query DSL labels Feb 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Function Score: Add optional weight parameter per function #7137

Function Score: Add optional weight parameter per function #7137

brwe commented Aug 3, 2014

clintongormley commented Aug 4, 2014

jpountz Aug 4, 2014

jpountz commented Aug 4, 2014

brwe commented Aug 11, 2014

jpountz Aug 12, 2014

brwe Aug 13, 2014

brwe Aug 13, 2014

brwe Aug 13, 2014

jpountz commented Aug 12, 2014

brwe commented Aug 13, 2014

brwe commented Aug 27, 2014

rjernst Aug 27, 2014

brwe Aug 29, 2014

brwe commented Aug 29, 2014

jpountz Aug 29, 2014

jpountz commented Aug 29, 2014

brwe commented Sep 1, 2014

Function Score: Add optional weight parameter per function #7137

Function Score: Add optional weight parameter per function #7137

Conversation

brwe commented Aug 3, 2014

clintongormley commented Aug 4, 2014

Choose a reason for hiding this comment

jpountz commented Aug 4, 2014

brwe commented Aug 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpountz commented Aug 12, 2014

brwe commented Aug 13, 2014

brwe commented Aug 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brwe commented Aug 29, 2014

Choose a reason for hiding this comment

jpountz commented Aug 29, 2014

brwe commented Sep 1, 2014