-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
Adding MinScore support to Linear Retriever #124182
Conversation
WIP - The testcases are failing, i have been trying to fix it. Also need to write yaml tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice start @mridula-s109
I left some comments. Additionally, it looks like you have some test compilation errors. I'd also love to see some yaml tests here, and we'll want to update our documentation to reflect the min score (I'm unsure of the state of our markdown documentation, but if we could do it in the same PR it would be ideal).
@@ -45,9 +47,11 @@ public RankDocsQueryBuilder(StreamInput in) throws IOException { | |||
if (in.getTransportVersion().onOrAfter(TransportVersions.V_8_16_0)) { | |||
this.queryBuilders = in.readOptionalArray(c -> c.readNamedWriteable(QueryBuilder.class), QueryBuilder[]::new); | |||
this.onlyRankDocs = in.readBoolean(); | |||
this.minScore = in.readFloat(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is written, it will break transport serialization.
What you want to do, is register a new TransportVersion
for your change in TransportVersions.java
. You'll then reference this new transport version to serialize the min score - older transport versions will always just use a default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If i want to include min_score without transport versioning changes, then is the way it is currently written fine?
server/src/main/java/org/elasticsearch/index/query/RankDocsQueryBuilder.java
Outdated
Show resolved
Hide resolved
@@ -88,6 +92,7 @@ protected void doWriteTo(StreamOutput out) throws IOException { | |||
if (out.getTransportVersion().onOrAfter(TransportVersions.V_8_16_0)) { | |||
out.writeOptionalArray(StreamOutput::writeNamedWriteable, queryBuilders); | |||
out.writeBoolean(onlyRankDocs); | |||
out.writeFloat(minScore); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to reference the new transport version you create here as well, to ensure that serialization is consistent.
server/src/main/java/org/elasticsearch/search/retriever/KnnRetrieverBuilder.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/retriever/KnnRetrieverBuilder.java
Outdated
Show resolved
Hide resolved
...lugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverBuilder.java
Outdated
Show resolved
Hide resolved
...lugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverBuilder.java
Show resolved
Hide resolved
...plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/linear/MinMaxScoreNormalizer.java
Outdated
Show resolved
Hide resolved
...plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/linear/MinMaxScoreNormalizer.java
Outdated
Show resolved
Hide resolved
...rf/src/test/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverBuilderParsingTests.java
Outdated
Show resolved
Hide resolved
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
Hi @mridula-s109, I've created a changelog YAML for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, LGTM. Left some comments
server/src/main/java/org/elasticsearch/index/query/RankDocsQueryBuilder.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/query/RankDocsQueryBuilder.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/retriever/KnnRetrieverBuilder.java
Outdated
Show resolved
Hide resolved
...plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/linear/10_linear_retriever.yml
Outdated
Show resolved
Hide resolved
...plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/linear/10_linear_retriever.yml
Show resolved
Hide resolved
Pinging @elastic/search-eng (Team:SearchOrg) |
Pinging @elastic/search-relevance (Team:Search - Relevance) |
server/src/main/java/org/elasticsearch/index/query/RankDocsQueryBuilder.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/query/RankDocsQueryBuilder.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/retriever/rankdoc/RankDocsQuery.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/retriever/rankdoc/RankDocsQuery.java
Outdated
Show resolved
Hide resolved
...-rrf/src/internalClusterTest/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverIT.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/query/RankDocsQueryBuilder.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/retriever/KnnRetrieverBuilder.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/retriever/KnnRetrieverBuilder.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/query/RankDocsQueryBuilderTests.java
Outdated
Show resolved
Hide resolved
...lugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverBuilder.java
Outdated
Show resolved
Hide resolved
...lugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverBuilder.java
Outdated
Show resolved
Hide resolved
...plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/linear/MinMaxScoreNormalizer.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/rrf/RRFRankDoc.java
Outdated
Show resolved
Hide resolved
...plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/linear/10_linear_retriever.yml
Show resolved
Hide resolved
x-pack/plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/rrf/RRFRetrieverBuilder.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/linear/LinearRankDoc.java
Outdated
Show resolved
Hide resolved
Closing this PR in favor of a fresh one: #126238 — will be published shortly. All feedback and comments from this PR will be carried forward into the new, cleaner version. Thanks for all the input so far! |
This PR introduces support for the min_score parameter in the linear retriever. The min_score filter ensures that only documents with a relevance score above a specified threshold are retrieved, improving result quality and filtering out low-relevance matches.