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

[Backport 2.x] Add positive_score_impact support for rank_features #2968

Merged
merged 1 commit into from
Apr 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
import org.opensearch.search.lookup.SearchLookup;

import java.io.IOException;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.function.Supplier;
Expand All @@ -55,8 +54,18 @@ public class RankFeaturesFieldMapper extends ParametrizedFieldMapper {

public static final String CONTENT_TYPE = "rank_features";

private static RankFeaturesFieldType ft(FieldMapper in) {
return ((RankFeaturesFieldMapper) in).fieldType();
}

public static class Builder extends ParametrizedFieldMapper.Builder {

private final Parameter<Boolean> positiveScoreImpact = Parameter.boolParam(
"positive_score_impact",
false,
m -> ft(m).positiveScoreImpact,
true
);
private final Parameter<Map<String, String>> meta = Parameter.metaParam();

public Builder(String name) {
Expand All @@ -66,16 +75,17 @@ public Builder(String name) {

@Override
protected List<Parameter<?>> getParameters() {
return Collections.singletonList(meta);
return List.of(meta, positiveScoreImpact);
}

@Override
public RankFeaturesFieldMapper build(BuilderContext context) {
return new RankFeaturesFieldMapper(
name,
new RankFeaturesFieldType(buildFullName(context), meta.getValue()),
new RankFeaturesFieldType(buildFullName(context), meta.getValue(), positiveScoreImpact.getValue()),
multiFieldsBuilder.build(this, context),
copyTo.build()
copyTo.build(),
positiveScoreImpact.getValue()
);
}
}
Expand All @@ -84,16 +94,23 @@ public RankFeaturesFieldMapper build(BuilderContext context) {

public static final class RankFeaturesFieldType extends MappedFieldType {

public RankFeaturesFieldType(String name, Map<String, String> meta) {
private final boolean positiveScoreImpact;

public RankFeaturesFieldType(String name, Map<String, String> meta, boolean positiveScoreImpact) {
super(name, false, false, false, TextSearchInfo.NONE, meta);
setIndexAnalyzer(Lucene.KEYWORD_ANALYZER);
this.positiveScoreImpact = positiveScoreImpact;
}

@Override
public String typeName() {
return CONTENT_TYPE;
}

public boolean positiveScoreImpact() {
return positiveScoreImpact;
}

@Override
public Query existsQuery(QueryShardContext context) {
throw new IllegalArgumentException("[rank_features] fields do not support [exists] queries");
Expand All @@ -115,9 +132,18 @@ public Query termQuery(Object value, QueryShardContext context) {
}
}

private RankFeaturesFieldMapper(String simpleName, MappedFieldType mappedFieldType, MultiFields multiFields, CopyTo copyTo) {
private final boolean positiveScoreImpact;

private RankFeaturesFieldMapper(
String simpleName,
MappedFieldType mappedFieldType,
MultiFields multiFields,
CopyTo copyTo,
boolean positiveScoreImpact
) {
super(simpleName, mappedFieldType, multiFields, copyTo);
assert fieldType.indexOptions().compareTo(IndexOptions.DOCS_AND_FREQS) <= 0;
this.positiveScoreImpact = positiveScoreImpact;
}

@Override
Expand Down Expand Up @@ -164,6 +190,9 @@ public void parse(ParseContext context) throws IOException {
+ "] in the same document"
);
}
if (positiveScoreImpact == false) {
value = 1 / value;
}
context.doc().addWithKey(key, new FeatureField(name(), feature, value));
} else {
throw new IllegalArgumentException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ protected void minimalMapping(XContentBuilder b) throws IOException {
}

@Override
protected void registerParameters(ParameterChecker checker) {
// no parameters to configure
protected void registerParameters(ParameterChecker checker) throws IOException {
checker.registerConflictCheck("positive_score_impact", b -> b.field("positive_score_impact", false));
}

@Override
Expand All @@ -95,6 +95,33 @@ public void testDefaults() throws Exception {
assertTrue(freq1 < freq2);
}

public void testNegativeScoreImpact() throws Exception {
DocumentMapper mapper = createDocumentMapper(
fieldMapping(b -> b.field("type", "rank_features").field("positive_score_impact", false))
);

ParsedDocument doc1 = mapper.parse(source(this::writeField));

IndexableField[] fields = doc1.rootDoc().getFields("field");
assertEquals(2, fields.length);
assertThat(fields[0], Matchers.instanceOf(FeatureField.class));
FeatureField featureField1 = null;
FeatureField featureField2 = null;
for (IndexableField field : fields) {
if (field.stringValue().equals("foo")) {
featureField1 = (FeatureField) field;
} else if (field.stringValue().equals("bar")) {
featureField2 = (FeatureField) field;
} else {
throw new UnsupportedOperationException();
}
}

int freq1 = RankFeatureFieldMapperTests.getFrequency(featureField1.tokenStream(null, null));
int freq2 = RankFeatureFieldMapperTests.getFrequency(featureField2.tokenStream(null, null));
assertTrue(freq1 > freq2);
}

public void testRejectMultiValuedFields() throws MapperParsingException, IOException {
DocumentMapper mapper = createDocumentMapper(mapping(b -> {
b.startObject("field").field("type", "rank_features").endObject();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
public class RankFeaturesFieldTypeTests extends FieldTypeTestCase {

public void testIsNotAggregatable() {
MappedFieldType fieldType = new RankFeaturesFieldMapper.RankFeaturesFieldType("field", Collections.emptyMap());
MappedFieldType fieldType = new RankFeaturesFieldMapper.RankFeaturesFieldType("field", Collections.emptyMap(), true);
assertFalse(fieldType.isAggregatable());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -157,3 +157,24 @@ setup:

- match:
hits.hits.1._id: "1"

---
"Negative linear":

- do:
search:
index: test
body:
query:
rank_feature:
field: url_length
linear: {}

- match:
hits.total.value: 2

- match:
hits.hits.0._id: "2"

- match:
hits.hits.1._id: "1"
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ setup:
properties:
tags:
type: rank_features
negative_reviews:
type: rank_features
positive_score_impact: false

- do:
index:
Expand All @@ -18,6 +21,9 @@ setup:
tags:
foo: 3
bar: 5
negative_reviews:
1star: 10
2star: 1

- do:
index:
Expand All @@ -27,6 +33,9 @@ setup:
tags:
bar: 6
quux: 10
negative_reviews:
1star: 1
2star: 10

- do:
indices.refresh: {}
Expand Down Expand Up @@ -97,3 +106,34 @@ setup:

- match:
hits.hits.1._id: "1"

---
"Linear negative impact":

- do:
search:
index: test
body:
query:
rank_feature:
field: negative_reviews.1star
linear: {}

- match:
hits.hits.0._id: "2"
- match:
hits.hits.1._id: "1"

- do:
search:
index: test
body:
query:
rank_feature:
field: negative_reviews.2star
linear: {}

- match:
hits.hits.0._id: "1"
- match:
hits.hits.1._id: "2"