Named queries should return their internal score as well

[synhershko]

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

• [X] New functionality includes testing.
  • [X] 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
• [X] Commits are signed per the DCO using --signoff
• [X] 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[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/16718/
• CommitID: 714d80247cbd0620335f37e40d124870d37c5195 Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/16719/
• CommitID: f59816f67fbb618a03062eb5de17a2ee13241edd Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/16720/
• CommitID: 30d13ef937c51f1bb7130c5e04f79fc38ad7bf47 Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/16725/
• CommitID: 5b35d5cda5fd9e60f17b25bc5c50e316bd9b226e Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/16729/
• CommitID: 78691845ead2a851d48cdc07f12a56fe17d0262b Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/16797/
• CommitID: e715622a405c8d7e10e8e4b28dc212b310a3b65a Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/16798/
• CommitID: 9d4227fd53d065c3a21bd2859fe0dd8b78023061 Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/16800/
• CommitID: 80bb5e65f31ff34e6ed6a41ee80124856568e2b2 Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/16802/
• CommitID: 02d04b6e6757bc3f1d36c14395e15b941544411c Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[synhershko]

Thanks! LGTM, iterate to green ... do you have an example of before/after response?

Of course. Current behavior of OpenSearch when named queries are enabled, is to return an array like this:

         "hits" : [
           {
              "_index" : "test",
              "_id" : "2",
              "_score" : 1.6207318,
              "_source" : {
                "foo" : "bar",
                "foo2" : "bar2"
              },
              "matched_queries" : ["foo2_query", "foo_query"]
            },
            {
              "_index" : "test",
              "_id" : "1",
              "_score" : 0.18232156,
              "_source" : {
                "foo" : "bar"
              },
              "matched_queries" : ["foo_query"]
            }
          ]

This PR changes the behavior so an object (query name -> score) is returned instead:

         "hits" : [
           {
              "_index" : "test",
              "_id" : "2",
              "_score" : 1.6207318,
              "_source" : {
                "foo" : "bar",
                "foo2" : "bar2"
              },
              "matched_queries" : {
                "foo2_query" : 1.4384103,
                "foo_query" : 0.2876821
              }
            },
            {
              "_index" : "test",
              "_id" : "1",
              "_score" : 0.18232156,
              "_source" : {
                "foo" : "bar"
              },
              "matched_queries" : {
                "foo_query" : 0.2876821
              }
            }
          ]

As indicated in the PR description, backwards compatibility tests fail and I'm waiting your guidance as to what should be expected here since the response format did change when named queries are enabled

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/17007/
• CommitID: 4dbf0ad87b024dfc354d967bbef2c267cf1a459f Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[dblock]

These are the days I wish this was a GraphQL interface :(

1. Should we make this future-compatible with additional fields?

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

2. As is it's a breaking change. We can make it in 3.0, but we would rather not in order to be able to backport the change into 2.x and ship it earlier, and to avoid breaking a bunch of clients. Maybe we can supply a new option into the named query, or a query parameter, e.g. matched_queries: showmeallthecoolinfoyougot? Is there similar precedent anywhere else? @macohen might know, too.

[synhershko]

Should we make this future-compatible with additional fields?

What additional fields? :)

As is it's a breaking change. We can make it in 3.0, but we would rather not in order to be able to backport the change into 2.x and ship it earlier, and to avoid breaking a bunch of clients.

I'm all in for releasing early - we have a customer waiting for that to be merged in :) Fixing wire backwards compatibility is easy inter-node, but clients will break because you can't touch older versions.

What I'd recommend then is using a feature flag, only returning score if named queries was requested and a query parameter (or other form of passing this flag) was passed asking for scores too. That'd mean clients will need to be asked to add support for this flag starting at the release version. WDYT?

[dblock]

Should we make this future-compatible with additional fields?

What additional fields? :)

So let's do it? It also makes clear what this numeric value is.

As is it's a breaking change. We can make it in 3.0, but we would rather not in order to be able to backport the change into 2.x and ship it earlier, and to avoid breaking a bunch of clients.

I'm all in for releasing early - we have a customer waiting for that to be merged in :) Fixing wire backwards compatibility is easy inter-node, but clients will break because you can't touch older versions.

I know we all love our customers, but let's not create more work for others.

What I'd recommend then is using a feature flag, only returning score if named queries was requested and a query parameter (or other form of passing this flag) was passed asking for scores too. That'd mean clients will need to be asked to add support for this flag starting at the release version. WDYT?

The problem with a feature flag is that it's not a permanent solution and we have dozens of clients. Can we try to at least discuss designing something durable and backwards compatible? Want to suggest a query addition and look for other cases where we have something like this?

[synhershko]

Can we try to at least discuss designing something durable and backwards compatible? Want to suggest a query addition and look for other cases where we have something like this?

Sure I'm all ears.

I think if feature flag is not an option, then the next best route here is to support named queries (_name) and named with score (_named_with_score).

I don't think you can escape the fact all clients will need to be updated with this new feature but at least we don't break them?

[dblock]

So what you're suggesting is deprecating matched_queries in 2.x and potentially removing it in 3.0? I like it. We could do this that I think can stand the test of time:

"matched" : {
  "queries" : {
     "foo_query" : {
       "score" : 123
    }
 }
}

Curious whether it's an overkill? @andrross @reta wdyt?

[synhershko]

So what you're suggesting is deprecating matched_queries in 2.x and potentially removing it in 3.0? I like it.

No, I think we should keep in line with the old response format - named queries are with us for many years and I think we should keep it the same as it was before. I woudln't deprecate, just find a way to add those scores to either the same response format, or by adding another field in the response.

[reta]

Curious whether it's an overkill? @andrross @reta wdyt?

I like the your previous idea that adds some properties to the named queries:

"matched_queries" : {
     "foo_query" : {
        "_score": 0.2876821
    }
}

But we definitely could not change the response without explicit opt-in from user. I am thinking what if we add the property to the search request, for example track_matched_query_scores (default false). Once set to true, we could return this new object (instead of just array of names).

[synhershko]

+1 for using a query string parameter as a feature flag, we can support it as a property too

[reta]

+1 for using a query string parameter as a feature flag, we can support it as a property too

@dblock wdyt?

[andrross]

+1 for using a query string parameter as a feature flag, we can support it as a property too

@dblock wdyt?

@reta @synhershko I'm not opposed to this, but I would rename the query string parameter to something like detailed_matched_queries or verbose_matched_queries. track_matched_query_scores would be obsolete/confusing if additional details are added.

[andrross]

Should we make this future-compatible with additional fields?

What additional fields? :)

So let's do it? It also makes clear what this numeric value is.

@dblock @synhershko I'm definitely in favor of the more verbose format simply because it is self-documenting with a score label, even if we never add additional fields.

[synhershko]

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]

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]

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?

[opensearch-trigger-bot[bot]]

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[bot]]

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

[kotwanikunal]

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[bot]]

Compatibility status:

Checks if related components are compatible with change 4dbf0ad

Incompatible components

Skipped components

Compatible components

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/25625/
• CommitID: 4dbf0ad87b024dfc354d967bbef2c267cf1a459f Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?