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

Named queries should return their internal score as well #7883

Closed

Conversation

synhershko
Copy link

@synhershko synhershko commented Jun 2, 2023

Description

While some internal queries may not be scored or not contrbute to the final doc score, in some cases (such as knn queries compbined with other queries) the distance metric is very interesting and should be reported back as part of the response.

This change adds that by default, so named queries always return the score (instead of just the name). It adds no additional cost.

Check List

  • New functionality includes testing.
    • All tests pass
    • Backwards compatibility tests fail - let's have a discussion about what should be expected here since the response format did change when named queries are enabled
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

Gradle Check (Jenkins) Run Completed with:

@synhershko synhershko force-pushed the scores-in-named-queries branch from 30d13ef to 5b35d5c Compare June 2, 2023 13:38
@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

Gradle Check (Jenkins) Run Completed with:

@synhershko synhershko force-pushed the scores-in-named-queries branch from 5b35d5c to 7869184 Compare June 2, 2023 15:29
@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

Gradle Check (Jenkins) Run Completed with:

@synhershko synhershko force-pushed the scores-in-named-queries branch from 7869184 to e715622 Compare June 4, 2023 05:47
@github-actions
Copy link
Contributor

github-actions bot commented Jun 4, 2023

Gradle Check (Jenkins) Run Completed with:

@synhershko
Copy link
Author

I'm just wondering what additional details we can provide there in the future? I'm calling YAGNI on it and let's do something simple that works first and worry about the rest later. I just don't see anything else we can possible want to provide other than score.

@macohen
Copy link
Contributor

macohen commented Jun 16, 2023

I understand why we would want the query string parameter here, but, to answer @dblock's question for me, I don't know of any precedent for this in OpenSearch. Even the query_string function is all in the body of the request and I really like that about OpenSearch (as a former old-school Solr user). I don't have a better answer for this either that doesn't create a breaking change to push this to 3.0 unfortunately.

@dblock
Copy link
Member

dblock commented Jun 20, 2023

Let's make a breaking change for 3.0 in this PR? I feel pretty strongly that we should avoid this problem for future changes here and do:

"matched_queries" : {
     "foo_query" : {
        "score": 0.2876821
    }
}

and not

"matched_queries" : {
     "foo_query" : 0.2876821
}

We can discuss a non-breaking version on a backport?

@macohen macohen added the Search Search query, autocomplete ...etc label Jun 22, 2023
@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity. Remove stalled label or comment or this will be closed in 7 days.

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label Jul 23, 2023
@opensearch-trigger-bot
Copy link
Contributor

This PR was closed because it has been stalled for 7 days with no activity.

@kotwanikunal
Copy link
Member

Apologies. This PR was auto closed without reaching a resolution from the maintainers.
Re-opening to move it forward.
Thanks for your contributions to OpenSearch!

@github-actions
Copy link
Contributor

Compatibility status:

Checks if related components are compatible with change 4dbf0ad

Incompatible components

Skipped components

Compatible components

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@synhershko
Copy link
Author

Thanks @kotwanikunal looking forward to final decision, let's get this in!

@ticheng-aws
Copy link
Contributor

Hi @synhershko, the PR is stalled. Is this being worked upon? Feel free to reach out to maintainers for further reviews.

@synhershko
Copy link
Author

@ticheng-aws I'm well aware this is stalled, see my comment #7883 (comment) and previous discussions. When maintainers pick it up and conclude their discussion I'm happy to pick it up again.

@synhershko
Copy link
Author

cc @dblock @macohen

@ticheng-aws ticheng-aws added the discuss Issues intended to help drive brainstorming and decision making label Jan 7, 2024
@dblock
Copy link
Member

dblock commented Jan 9, 2024

I think #7883 (comment) is still accurate? A breaking change for main, do we need more maintainers to pitch in? WDYT @msfroh / @andrross?

@opensearch-trigger-bot opensearch-trigger-bot bot removed the stalled Issues that have stalled label Jan 14, 2024
@andrross
Copy link
Member

I think #7883 (comment) is still accurate? A breaking change for main, do we need more maintainers to pitch in?

@dblock I agree with your suggestion. I'll ship this PR as a breaking change to main with the structure you proposed. I think we can deliver this in 2.x with some sort of yet-to-be-decided opt-in mechanism as well, but we can work that out once this has landed in main.

@reta
Copy link
Collaborator

reta commented Feb 15, 2024

@dblock @andrross I believe the feature is already integrated #11626

@dblock
Copy link
Member

dblock commented Feb 27, 2024

@synhershko take a look? what shall we do with this PR?

@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label Apr 28, 2024
@andrross
Copy link
Member

andrross commented May 6, 2024

I believe this has been implemented in #11626. I'm going to close this. @synhershko Please do speak up if #11626 doesn't solve your problem.

@andrross andrross closed this May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues intended to help drive brainstorming and decision making Search Search query, autocomplete ...etc stalled Issues that have stalled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants