Add positive_score_impact support for rank_features
#2725
Conversation
Signed-off-by: Yevhen Tienkaiev <hronom@gmail.com>
|
❌ Gradle Check failure 4a953f8798ce4e6bf8be9d112b7d1d4ee20ff57e |
|
❌ Gradle Check failure 564f5759c33ed28f35353e5dbff290e97c122b15 |
| @@ -66,16 +76,20 @@ public Builder(String name) { | |||
|
|
|||
| @Override | |||
| protected List<Parameter<?>> getParameters() { | |||
| return Collections.singletonList(meta); | |||
| ArrayList<Parameter<?>> result = new ArrayList<>(2); | |||
There was a problem hiding this comment.
| ArrayList<Parameter<?>> result = new ArrayList<>(2); | |
| 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()), |
There was a problem hiding this comment.
I see it being used by final Parameter<Boolean> positiveScoreImpact, the RankFeaturesFieldType could drop positiveScoreImpact and use it from RankFeaturesFieldType since it has a direct reference to it (sorry, copied from #2726 (comment))
There was a problem hiding this comment.
@reta I just follow same style like it's done in:
org.opensearch.index.mapper.RankFeatureFieldMapperorg.opensearch.index.mapper.ScaledFloatFieldMapper
They also have it in Builders.
Also you can check that there meta on the same level and it's have same situation:
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();
I think it's better to be in the same style for now and if needed do refactoring for all places in separate PR.
However, I'm ok if maintainers of this repository do changes that is needed before merging this PR.
There was a problem hiding this comment.
👍 no objection from be, just feels a bit redundant
|
Hi @Hronom! Thank you so much for submitting this! I was about to ask if you wanted to finish up the last few comments but saw you're in Ukraine. Our thoughts are with you and your family during these difficult times. We are happy to tidy up these last few comments for you and commit on your behalf. Thank you so much for finding the time to contribute code to the project. We are extremely grateful! |
|
@nknize thank you! I will try to add yours feedbacks now |
Signed-off-by: Yevhen Tienkaiev <hronom@gmail.com>
|
Also feel free to make adjustments in this PR on my behalf, you have my approve for this! |
|
Please check also this update to documentation opensearch-project/documentation-website#500 I think it will be good to make more people know about it. |
nknize
added
enhancement
Adds positive_score_impact support for rank_features field mapper. Signed-off-by: Yevhen Tienkaiev <hronom@gmail.com> (cherry picked from commit d8c815c)
Adds positive_score_impact support for rank_features field mapper. Signed-off-by: Yevhen Tienkaiev <hronom@gmail.com> (cherry picked from commit d8c815c) Co-authored-by: Yevhen Tienkaiev <hronom@gmail.com>
Description
Added support for
positive_score_impactparameter forrank_featuresIssues Resolved
[List any issues this PR will resolve]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.