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

Add positive_score_impact support for rank_features #2726

Closed
wants to merge 4 commits into from
Closed

Add positive_score_impact support for rank_features #2726

wants to merge 4 commits into from

Conversation

Hronom
Copy link
Contributor

@Hronom Hronom commented Apr 3, 2022

Description

Added support for positive_score_impact parameter for rank_features

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

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.

Hronom added 3 commits July 30, 2021 02:36
Signed-off-by: Yevhen Tienkaiev <hronom@gmail.com>
Signed-off-by: Yevhen Tienkaiev <hronom@gmail.com>
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure f50be02
Log 4083

Reports 4083

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 435ea51
Log 4084

Reports 4084

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 7d46d7610b431248a2fa850ce48289e8f99d78d3
Log 4086

Reports 4086

Signed-off-by: Yevhen Tienkaiev <hronom@gmail.com>
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 0b1326f7d846e8457cc9a053a51e1dcaa64c86c7
Log 4088

Reports 4088

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure cf7bdc3358fb34d7cdaee10c78d04f8fa48b0c0e
Log 4089

Reports 4089

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 2bd2e8d
Log 4090

Reports 4090

@Hronom
Copy link
Contributor Author

Hronom commented Apr 4, 2022

start gradle check

@Hronom
Copy link
Contributor Author

Hronom commented Apr 4, 2022

Admins please restart build... since failed test completed ok locally and changes also works good here #2725

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 2bd2e8d
Log 4091

Reports 4091

Copy link
Collaborator

@nknize nknize left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will not be releasing 1.4 and 2.0 is feature frozen so the earliest we can merge this will be for 2.1. Let's work on getting it in main first and then we'll autobackport to 2.x.

@@ -84,16 +98,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;
Copy link
Collaborator

@reta reta Apr 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like positiveScoreImpact is not needed for RankFeaturesFieldType, the positiveScoreImpact() is never used

Apologies, 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.

}

@Override
public RankFeaturesFieldMapper build(BuilderContext context) {
return new RankFeaturesFieldMapper(
name,
new RankFeaturesFieldType(buildFullName(context), meta.getValue()),
new RankFeaturesFieldType(buildFullName(context), meta.getValue(), positiveScoreImpact.getValue()),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like either RankFeaturesFieldMapper or RankFeaturesFieldType need positiveScoreImpact, but not both (see please https://github.com/opensearch-project/OpenSearch/pull/2726/files#r841752169)

@nknize
Copy link
Collaborator

nknize commented Apr 4, 2022

Please fold the above PR feedback into #2725. We will not be merging this into 1.x since there will be no 1.4 release.

@dblock
Copy link
Member

dblock commented Apr 11, 2022

I'll close this and leave #2725. Once in main we can talk about how far we want to backport it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants