Support null_value functionality for rank_feature field type(#95149) #95811
Conversation
elasticsearchmachine added v8.9.0 needs:triage 
dliappis added :Search Relevance/Ranking 
| Pinging @elastic/es-search (Team:Search) |
| @elasticmachine test this please |
...apper-extras/src/main/java/org/elasticsearch/index/mapper/extras/RankFeatureFieldMapper.java Outdated Show resolved Hide resolved
...apper-extras/src/main/java/org/elasticsearch/index/mapper/extras/RankFeatureFieldMapper.java Outdated Show resolved Hide resolved
...apper-extras/src/main/java/org/elasticsearch/index/mapper/extras/RankFeatureFieldMapper.java Outdated Show resolved Hide resolved
There was a problem hiding this comment.
@Kiriakos1998 Thanks for your contributions. Good progress so far.
I've left the initial comments. Can you please also fix formatting by running ./gradlew :modules:mapper-extras:spotlessApply
Kiriakos1998 force-pushed the null_value_rank_feature/95149 branch from f89f2ac to 937fbeb Compare
@mayya-sharipova Thanks for reviewing this PR. spotlessApply also done. |
...apper-extras/src/main/java/org/elasticsearch/index/mapper/extras/RankFeatureFieldMapper.java Show resolved Hide resolved
modules/mapper-extras/src/yamlRestTest/resources/rest-api-spec/test/rank_feature/10_basic.yml Outdated Show resolved Hide resolved
modules/mapper-extras/src/yamlRestTest/resources/rest-api-spec/test/rank_feature/10_basic.yml Outdated Show resolved Hide resolved
modules/mapper-extras/src/yamlRestTest/resources/rest-api-spec/test/rank_feature/10_basic.yml Outdated Show resolved Hide resolved
| public void testNegativeScoreImpact() throws Exception { | ||
| DocumentMapper mapper = createDocumentMapper( | ||
| fieldMapping(b -> b.field("type", "rank_feature").field("positive_score_impact", false)) | ||
| /** |
There was a problem hiding this comment.
These tests look very complex and unnecessary to my mind. Let's instead of them use something simple, similar to KeywordFieldMapperTests::testNullValue()
There was a problem hiding this comment.
@Kiriakos1998 Thanks for making progress on this. The main code looks very good to me (except correcting error message). Let's also adjust the tests to standard we have in the team.
| @elasticsearchmachine test this please |
Hello, I think I need to do one more commit because in the RankFeatureFieldMapperTests.java file Intellij replaced explicit import with wild *. |
Thank you. Before submitting your commits, please run locally |
Done |
| | ||
| --- | ||
| "Non positive null_vallue": | ||
| |
There was a problem hiding this comment.
It looks like setup section is not relevant for this test, as we are not doing any request agains test1 index. Should we get rid of setup section all togehter, and instead have two tests:
- "Non positive null_vallue"
- "Search rank_feature with and without no null_value", where we create and add docs to
test1and do 2 searches against it.
Each of this test will have its ownskipsection.
| } | ||
| | ||
| public void testNullValue() throws IOException { | ||
| DocumentMapper mapper = createDocumentMapper(fieldMapping(this::minimalMapping)); |
There was a problem hiding this comment.
Sorry, Lines 149-151 are not relevant for rank_feature field, which we need to explicitly define in mappings.
There was a problem hiding this comment.
Should I delete those lines or replace them with these lines of code?
DocumentMapper mapper = createDocumentMapper(fieldMapping(b -> b.field("type", "rank_feature"))); ParsedDocument doc = mapper.parse(source(b -> b.nullField("field"))); assertThat(doc.rootDoc().getFields("_feature"), empty());
There was a problem hiding this comment.
@Kiriakos1998 Thanks for iterating. The code looks perfect! We just need to do small changes in tests, and this PR will be ready!
@mayya-sharipova Thanks for the feedback. I have pushed the changes for the yaml rest test and the unit test. |
| @elasticsearchmachine test this please |
There was a problem hiding this comment.
@Kiriakos1998 Thanks for your work. PR looks good to me! Merged for Elasticsearch v.8.9.
...s/mapper-extras/src/yamlRestTest/resources/rest-api-spec/test/rank_feature/20_null_value.yml Show resolved Hide resolved
...apper-extras/src/main/java/org/elasticsearch/index/mapper/extras/RankFeatureFieldMapper.java Show resolved Hide resolved
Correct and add more tests for adding null_value parameter for the rank_feature field. Relates to elastic#95811, closes elastic#95149
5 participants
At the moment rank_feature does not support null_value functionality. So documents, with null as their value for rank_feature fields, can not be indexed and so they can not be retrieved when searching. This PR implements this functionality. So if the null_value is specified in a rank_feature field, all documents with null as their value for this rank_feature will use during indexing the value specified in null_value.
Closes(#95149)