Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Function Score: Add optional weight parameter per function #7137

Closed
wants to merge 18 commits into from

Conversation

Projects
None yet
4 participants
@brwe
Copy link
Contributor

brwe commented Aug 3, 2014

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

@brwe brwe added the review label Aug 3, 2014

@clintongormley

This comment has been minimized.

Copy link
Member

clintongormley commented Aug 4, 2014

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

@jpountz

View changes

docs/reference/query-dsl/queries/function-score-query.asciidoc Outdated
@@ -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.

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 4, 2014

Contributor

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

@jpountz

View changes

src/main/java/org/elasticsearch/common/lucene/search/function/BoostScoreFunction.java Outdated
public class BoostScoreFunction extends ScoreFunction {

public static final String BOOST_WEIGHT_ERROR_MESSAGE = "boost_factor/weight defined together does not make sense. Use one of them only. weight is preferable because boost_factor is deprecated. If you are using the java API, use either FactorBuilder#boostFactor(..) or a WeightFactorBuilder. WeightFactorBuilder is preferable because FactorBuilder is deprecated.";

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 4, 2014

Contributor

If a user gets this message, it means that they are using the Java API? ("if you are using the Java API")

@jpountz

View changes

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

WeightFactorFunction that = (WeightFactorFunction) o;

if (Double.compare(that.getWeight(), getWeight()) != 0)

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 4, 2014

Contributor

Why do you use Double.compare instead of ==/!= ?

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 4, 2014

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

This comment has been minimized.

Copy link
Contributor Author

brwe commented Aug 11, 2014

Thanks for the review! Implemented all suggestions.

@brwe brwe added the review label Aug 11, 2014

@jpountz

View changes

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

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

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 12, 2014

Contributor

Should it mention the wrapped function as well?

This comment has been minimized.

Copy link
@brwe

brwe Aug 13, 2014

Author Contributor

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"
                  }
               ]
            }

This comment has been minimized.

Copy link
@brwe

brwe Aug 13, 2014

Author Contributor

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

This comment has been minimized.

Copy link
@brwe

brwe Aug 13, 2014

Author Contributor

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

@jpountz

View changes

src/main/java/org/elasticsearch/index/query/functionscore/ScoreFunctionBuilder.java Outdated
if (weight != null) {
builder.field(FunctionScoreQueryParser.WEIGHT_FIELD.getPreferredName(), weight);
}
}

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 12, 2014

Contributor

I think it is a bit weird that if you want to serialize a ScoreFunctionBuilder, you need to call both toXContent and buildWeight. Maybe we could rather have something like:

  protected void buildWeight(XContentBuilder builder) throws IOException {
    if (weight != null) {
      builder.field(FunctionScoreQueryParser.WEIGHT_FIELD.getPreferredName(), weight);
    }
  }

  @Override
  public final void toXContent(XContentBuilder builder, Params params) {
    buildWeight(builder);
    doXContent(builder, params);
  }

  protected abstract void doXContent(XContentBuilder builder, Params params) throws IOException;

This way, toXContent would be all that needs to be called to serialize a ScoreFunction and the functions that want to override buildWeight could still do it?

/**
*
*/
public class WeightFactorFunction extends ScoreFunction {

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 12, 2014

Contributor

Given that score/explainScore/setNextReader need to check whether the function is null, maybe it would make sense to keep this class but assume that the function is never null, and add a new specialization that simply holds a weight? Or alternatively, when a null function is provided in this class replace it with a function that always returns 1 as a score?

This comment has been minimized.

Copy link
@brwe

brwe Aug 13, 2014

Author Contributor

Yes, changed in "insert score function that always scores 1.0 if no score function..."

@jpountz

View changes

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

@Override
public int hashCode() {
return (getWeight() != +0.0f ? Float.floatToIntBits((float) getWeight()) : 0);

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 12, 2014

Contributor

This is a weird hashCode? The int bits of +0.0f are 0, so this should be equivalent to return Float.floatToIntBits((float) getWeight()); I think?

This comment has been minimized.

Copy link
@brwe

brwe Aug 13, 2014

Author Contributor

Indeed, it is another leftover from copying BoostScoreFunction that I did not check. I removed equals() and hashCode() in WeightFactorFunction and BoostScoreFunction because it is not needed anywhere.

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 12, 2014

@brwe Left some more comments

@jpountz jpountz removed the review label Aug 12, 2014

@brwe

This comment has been minimized.

Copy link
Contributor Author

brwe commented Aug 13, 2014

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 brwe removed their assignment Aug 21, 2014

@brwe brwe force-pushed the brwe:issue-6955 branch 2 times, most recently Aug 27, 2014

@brwe

This comment has been minimized.

Copy link
Contributor Author

brwe commented Aug 27, 2014

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

@brwe brwe added the review label Aug 27, 2014

@rjernst

View changes

docs/reference/query-dsl/queries/function-score-query.asciidoc Outdated
@@ -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 comment has been minimized.

Copy link
@rjernst

rjernst Aug 27, 2014

Member

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

"but with additional capabilities such as"

This comment has been minimized.

Copy link
@brwe

brwe Aug 29, 2014

Author Contributor

yes, that's better

@rjernst

View changes

docs/reference/query-dsl/queries/function-score-query.asciidoc Outdated
===== Boost factor
===== Weight

added[1.4.0]

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 27, 2014

Member

I've learned recently that this should be "coming[1.4.0]" and then the release build process will change it to "added"?

This comment has been minimized.

Copy link
@brwe

brwe Aug 29, 2014

Author Contributor

neat! I did not know.

@rjernst

View changes

docs/reference/query-dsl/queries/function-score-query.asciidoc Outdated
@@ -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 `field_value_factor`) and also because sometimes a different impact of functions on the score is desirable, 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.

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 27, 2014

Member

I don't know if the "added" label can be in the middle of a sentence? Have you tried generating the docs to see what it looks like?

This comment has been minimized.

Copy link
@brwe

brwe Aug 29, 2014

Author Contributor

Looks like this (with tooltip, screenshot did not capture mouse position)

screen shot 2014-08-28 at 10 13 40 pm

@rjernst

View changes

docs/reference/query-dsl/queries/function-score-query.asciidoc Outdated
@@ -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 `field_value_factor`) and also because sometimes a different impact of functions on the score is desirable, 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.
If weight is given without any other function declaration, `weight` acts as a function that simply returns the `weight`.

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 27, 2014

Member

This seems like an odd feature to me. What boost_factor like this before? Could we instead give an error?

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 27, 2014

Member

You can ignore this comment, as I learned that this matches the existing behavior of boost_factor.

@rjernst

View changes

src/main/java/org/elasticsearch/common/lucene/search/function/BoostScoreFunction.java Outdated
public class BoostScoreFunction extends ScoreFunction {

public static final String BOOST_WEIGHT_ERROR_MESSAGE_PARSER = "boost_factor/weight defined together does not make sense. Use one of them only. weight is preferable because boost_factor is deprecated.";

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 27, 2014

Member

I think this message could be simplified to:
"'boost_factor' and 'weight' cannot be used together. Use 'weight'"

brwe added some commits Aug 29, 2014

@brwe brwe force-pushed the brwe:issue-6955 branch to f006bdd Aug 29, 2014

@brwe

This comment has been minimized.

Copy link
Contributor Author

brwe commented Aug 29, 2014

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

brwe added a commit to brwe/elasticsearch that referenced this pull request Aug 29, 2014

mappings: keep parameters in mapping for _timestamp, _index and _size…
… 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

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

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 29, 2014

Contributor

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

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 29, 2014

Just left one minor comment, other than that LGTM

@jpountz jpountz removed the review label Aug 29, 2014

brwe added a commit that referenced this pull request Sep 1, 2014

mappings: keep parameters in mapping for _timestamp, _index and _size…
… 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 added a commit that referenced this pull request Sep 1, 2014

mappings: keep parameters in mapping for _timestamp, _index and _size…
… 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 added a commit that referenced this pull request Sep 1, 2014

mappings: keep parameters in mapping for _timestamp, _index and _size…
… 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 brwe closed this in 9750375 Sep 1, 2014

@brwe brwe reopened this Sep 1, 2014

@brwe

This comment has been minimized.

Copy link
Contributor Author

brwe commented Sep 1, 2014

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

@brwe brwe closed this in c5ff70b Sep 1, 2014

brwe added a commit that referenced this pull request Sep 1, 2014

function_score: add optional weight parameter per function
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

brwe added a commit that referenced this pull request Sep 8, 2014

mappings: keep parameters in mapping for _timestamp, _index and _size…
… 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 added a commit that referenced this pull request Sep 8, 2014

function_score: add optional weight parameter per function
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

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

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015

mappings: keep parameters in mapping for _timestamp, _index and _size…
… 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

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015

mappings: keep parameters in mapping for _timestamp, _index and _size…
… 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.