Skip to content

Commit 03ba5b1

Browse files
mridula-s109tlrxmartijnvgbreskebynik9000
authored
Fix minscore propagation in text similarity reranker (elastic#129223)
* propgating retrievers to inner retrievers * test feature taken care of * Small changes in concurrent multipart upload interfaces (elastic#128977) Small changes in BlobContainer interface and wrapper. Relates ES-11815 * Unmute FollowingEngineTests#testProcessOnceOnPrimary() test (elastic#129054) The reason the test fails is that operations contained _seq_no field with different doc value types (with no skippers and with skippers) and this isn't allowed, since field types need to be consistent in a Lucene index. The initial operations were generated not knowing about the fact the index mode was set to logsdb or time_series. Causing the operations to not have doc value skippers. However when replaying the operations via following engine, the operations did have doc value skippers. The fix is to set `index.seq_no.index_options` to `points_and_doc_values`, so that the initial operations are indexed without doc value skippers. This test doesn't gain anything from storing seqno with doc value skippers, so there is no loss of testing coverage. Closes elastic#128541 * [Build] Add support for publishing to maven central (elastic#128659) This ensures we package an aggregation zip with all artifacts we want to publish to maven central as part of a release. Running zipAggregation will produce a zip file in the build/nmcp/zip folder. The content of this zip is meant to match the maven artifacts we have currently declared as dra maven artifacts. * ESQL: Check for errors while loading blocks (elastic#129016) Runs a sanity check after loading a block of values. Previously we were doing a quick check if assertions were enabled. Now we do two quick checks all the time. Better - we attach information about how a block was loaded when there's a problem. Relates to elastic#128959 * Make `PhaseCacheManagementTests` project-aware (elastic#129047) The functionality in `PhaseCacheManagement` was already project-aware, but these tests were still using deprecated methods. * Vector test tools (elastic#128934) This adds some testing tools for verifying vector recall and latency directly without having to spin up an entire ES node and running a rally track. Its pretty barebones and takes inspiration from lucene-util, but I wanted access to our own formats and tooling to make our lives easier. Here is an example config file. This will build the initial index, run queries at num_candidates: 50, then again at num_candidates 100 (without reindexing, and re-using the cached nearest neighbors). ``` [{ "doc_vectors" : "path", "query_vectors" : "path", "num_docs" : 10000, "num_queries" : 10, "index_type" : "hnsw", "num_candidates" : 50, "k" : 10, "hnsw_m" : 16, "hnsw_ef_construction" : 200, "index_threads" : 4, "reindex" : true, "force_merge" : false, "vector_space" : "maximum_inner_product", "dimensions" : 768 }, { "doc_vectors" : "path", "query_vectors" : "path", "num_docs" : 10000, "num_queries" : 10, "index_type" : "hnsw", "num_candidates" : 100, "k" : 10, "hnsw_m" : 16, "hnsw_ef_construction" : 200, "vector_space" : "maximum_inner_product", "dimensions" : 768 } ] ``` To execute: ``` ./gradlew :qa:vector:checkVec --args="/Path/to/knn_tester_config.json" ``` Calling `./gradlew :qa:vector:checkVecHelp` gives some guidance on how to use it, additionally providing a way to run it via java directly (useful to bypass gradlew guff). * ES|QL: refactor generative tests (elastic#129028) * Add a test of LOOKUP JOIN against a time series index (elastic#129007) Add a spec test of `LOOKUP JOIN` against a time series index. * Make ILM `ClusterStateWaitStep` project-aware (elastic#129042) This is part of an iterative process to make ILM project-aware. * Mute org.elasticsearch.xpack.esql.qa.mixed.MixedClusterEsqlSpecIT test {lookup-join.LookupJoinOnTimeSeriesIndex ASYNC} elastic#129078 * Remove `ClusterState` param from ILM `AsyncBranchingStep` (elastic#129076) The `ClusterState` parameter of the `asyncPredicate` is not used anywhere. * Mute org.elasticsearch.xpack.esql.qa.mixed.MixedClusterEsqlSpecIT test {lookup-join.LookupJoinOnTimeSeriesIndex SYNC} elastic#129082 * Mute org.elasticsearch.upgrades.UpgradeClusterClientYamlTestSuiteIT test {p0=upgraded_cluster/70_ilm/Test Lifecycle Still There And Indices Are Still Managed} elastic#129097 * Mute org.elasticsearch.upgrades.UpgradeClusterClientYamlTestSuiteIT test {p0=upgraded_cluster/90_ml_data_frame_analytics_crud/Get mixed cluster outlier_detection job} elastic#129098 * Mute org.elasticsearch.packaging.test.DockerTests test081SymlinksAreFollowedWithEnvironmentVariableFiles elastic#128867 * Threadpool merge executor is aware of available disk space (elastic#127613) This PR introduces 3 new settings: indices.merge.disk.check_interval, indices.merge.disk.watermark.high, and indices.merge.disk.watermark.high.max_headroom that control if the threadpool merge executor starts executing new merges when the disk space is getting low. The intent of this change is to avoid the situation where in-progress merges exhaust the available disk space on the node's local filesystem. To this end, the thread pool merge executor periodically monitors the available disk space, as well as the current disk space estimates required by all in-progress (currently running) merges on the node, and will NOT schedule any new merges if the disk space is getting low (by default below the 5% limit of the total disk space, or 100 GB, whichever is smaller (same as the disk allocation flood stage level)). * Add option to include or exclude vectors from _source retrieval (elastic#128735) This PR introduces a new include_vectors option to the _source retrieval context. When set to false, vectors are excluded from the returned _source. This is especially efficient when used with synthetic source, as it avoids loading vector fields entirely. By default, vectors remain included unless explicitly excluded. * Remove direct minScore propagation to inner retrievers * cleaned up skip * Mute org.elasticsearch.index.engine.ThreadPoolMergeExecutorServiceDiskSpaceTests testAvailableDiskSpaceMonitorWhenFileSystemStatErrors elastic#129149 * Add transport version for ML inference Mistral chat completion (elastic#129033) * Add transport version for ML inference Mistral chat completion * Add changelog for Mistral Chat Completion version fix * Revert "Add changelog for Mistral Chat Completion version fix" This reverts commit 7a57416. * Correct index path validation (elastic#129144) All we care about is if reindex is true or false. We shouldn't worry about force merge. Because if reindex is true, we will create the directory, if its false, we won't. * Mute org.elasticsearch.index.engine.ThreadPoolMergeExecutorServiceDiskSpaceTests testUnavailableBudgetBlocksNewMergeTasksFromStartingExecution elastic#129148 * Implemented completion task for Google VertexAI (elastic#128694) * Google Vertex AI completion model, response entity and tests * Fixed GoogleVertexAiServiceTest for Service configuration * Changelog * Removed downcasting and using `moveToFirstToken` * Create GoogleVertexAiChatCompletionResponseHandler for streaming and non streaming responses * Added unit tests * PR feedback * Removed googlevertexaicompletion model. Using just GoogleVertexAiChatCompletionModel for completion and chat completion * Renamed uri -> nonStreamingUri. Added streamingUri and getters in GoogleVertexAiChatCompletionModel * Moved rateLimitGroupHashing to subclasses of GoogleVertexAiModel * Fixed rate limit has of GoogleVertexAiRerankModel and refactored uri for GoogleVertexAiUnifiedChatCompletionRequest --------- Co-authored-by: lhoet-google <[email protected]> Co-authored-by: Jonathan Buttner <[email protected]> * Fixing minscore filtering in the text similarity reranker * ES|QL - kNN function initial support (elastic#127322) * Remove optional seed from ES|QL SAMPLE (elastic#128887) * Remove optional seed from ES|QL SAMPLE * make it clear that seed is for testing * [Inference API] Add "rerank" task type to "elastic" provider (elastic#126022) * Rename target destination for microbenchmarks (elastic#128878) * Include direct memory and non-heap memory in ML memory calculations (take #2) (elastic#128742) * Include direct memory and non-heap memory in ML memory calculations. * Reduce ML_ONLY heap size, so that direct memory is accounted for. * [CI] Auto commit changes from spotless * changelog * improve docs * Reuse direct memory to heap factor * feature flag --------- Co-authored-by: elasticsearchmachine <[email protected]> * Throw better exception for unsupported aggregations over shape fields (elastic#129139) * Update Test Framework To Handle Query Rewrites That Rely on Non-Null Searchers (elastic#129160) * Update ReproduceInfoPrinter to correctly print a reproduction line for Lucene & build candidate upgrade tests (elastic#129044) * Increment inference stats counter for shard bulk inference calls (elastic#129140) This change updates the inference stats counter to include chunked inference calls performed by the shard bulk inference filter on all semantic text fields. It ensures that usage of inference on semantic text fields is properly recorded in the stats. * Synthetic source: avoid storing multi fields of type text and match_only_text by default. (elastic#129126) Don't store text and match_only_text field by default when source mode is synthetic and a field is a multi field or when there is a suitable multi field. Without this change, ES would store field otherwise twice in a multi-field configuration. For example: ``` ... "os": { "properties": { "name": { "ignore_above": 1024, "type": "keyword", "fields": { "text": { "type": "match_only_text" } } } ... ``` In this case, two stored fields were added, one in case for the `name` field and one for `name.text` multi-field. This change prevents this, and would never store a stored field when text or match_only_text field is a multi-field. * Adding `scheduled_report_id` field to kibana reporting template (elastic#127827) * Adding scheduled_report_id field to kibana reporting template * Incrementing stack template registry version * ES|QL: Add FORK generative tests (elastic#129135) * ES|QL Completion command syntax change (elastic#129189) * propagated minscore to rankdsocsretrieverbuilder * Modified the file to include minscore and the test case to verify it * Revert "Use IndexOrDocValuesQuery in NumberFieldType#termQuery implementations (elastic#128293)" (elastic#129206) This reverts commit de7c91c. * Fixed the rankdocsretriever builder * Update docs/changelog/129223.yaml * Update 129223.yaml * trying to introduce cluster featureS * included cluster features in the test * Fixed the merge issue * [CI] Auto commit changes from spotless * Removed local variable from RankDocsRetrieverBuilder * Update RankDocsRetrieverBuilder.java --------- Co-authored-by: Tanguy Leroux <[email protected]> Co-authored-by: Martijn van Groningen <[email protected]> Co-authored-by: Rene Groeschke <[email protected]> Co-authored-by: Nik Everett <[email protected]> Co-authored-by: Niels Bauman <[email protected]> Co-authored-by: Benjamin Trent <[email protected]> Co-authored-by: Luigi Dell'Aquila <[email protected]> Co-authored-by: Bogdan Pintea <[email protected]> Co-authored-by: elasticsearchmachine <[email protected]> Co-authored-by: Albert Zaharovits <[email protected]> Co-authored-by: Jim Ferenczi <[email protected]> Co-authored-by: Jan-Kazlouski-elastic <[email protected]> Co-authored-by: Leonardo Hoet <[email protected]> Co-authored-by: lhoet-google <[email protected]> Co-authored-by: Jonathan Buttner <[email protected]> Co-authored-by: Carlos Delgado <[email protected]> Co-authored-by: Jan Kuipers <[email protected]> Co-authored-by: Tim Grein <[email protected]> Co-authored-by: Ievgen Degtiarenko <[email protected]> Co-authored-by: elasticsearchmachine <[email protected]> Co-authored-by: Ignacio Vera <[email protected]> Co-authored-by: Mike Pellegrini <[email protected]> Co-authored-by: Moritz Mack <[email protected]> Co-authored-by: Ying Mao <[email protected]> Co-authored-by: Ioana Tagirta <[email protected]> Co-authored-by: Aurélien FOUCRET <[email protected]>
1 parent e62e59c commit 03ba5b1

File tree

7 files changed

+133
-17
lines changed

7 files changed

+133
-17
lines changed

docs/changelog/129223.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 129223
2+
summary: Fix text similarity reranker does not propagate min score correctly
3+
area: Search
4+
type: bug
5+
issues: []

server/src/main/java/org/elasticsearch/search/retriever/CompoundRetrieverBuilder.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,8 @@ public void onFailure(Exception e) {
195195
RankDocsRetrieverBuilder rankDocsRetrieverBuilder = new RankDocsRetrieverBuilder(
196196
rankWindowSize,
197197
newRetrievers.stream().map(s -> s.retriever).toList(),
198-
results::get
198+
results::get,
199+
this.minScore
199200
);
200201
rankDocsRetrieverBuilder.retrieverName(retrieverName());
201202
return rankDocsRetrieverBuilder;

server/src/main/java/org/elasticsearch/search/retriever/RankDocsRetrieverBuilder.java

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,13 +33,14 @@ public class RankDocsRetrieverBuilder extends RetrieverBuilder {
3333
final List<RetrieverBuilder> sources;
3434
final Supplier<RankDoc[]> rankDocs;
3535

36-
public RankDocsRetrieverBuilder(int rankWindowSize, List<RetrieverBuilder> sources, Supplier<RankDoc[]> rankDocs) {
36+
public RankDocsRetrieverBuilder(int rankWindowSize, List<RetrieverBuilder> sources, Supplier<RankDoc[]> rankDocs, Float minScore) {
3737
this.rankWindowSize = rankWindowSize;
3838
this.rankDocs = rankDocs;
3939
if (sources == null || sources.isEmpty()) {
4040
throw new IllegalArgumentException("sources must not be null or empty");
4141
}
4242
this.sources = sources;
43+
this.minScore = minScore;
4344
}
4445

4546
@Override
@@ -48,7 +49,7 @@ public String getName() {
4849
}
4950

5051
private boolean sourceHasMinScore() {
51-
return minScore != null || sources.stream().anyMatch(x -> x.minScore() != null);
52+
return this.minScore != null || sources.stream().anyMatch(x -> x.minScore() != null);
5253
}
5354

5455
private boolean sourceShouldRewrite(QueryRewriteContext ctx) throws IOException {
@@ -132,7 +133,7 @@ public void extractToSearchSourceBuilder(SearchSourceBuilder searchSourceBuilder
132133
searchSourceBuilder.size(rankWindowSize);
133134
}
134135
if (sourceHasMinScore()) {
135-
searchSourceBuilder.minScore(this.minScore() == null ? Float.MIN_VALUE : this.minScore());
136+
searchSourceBuilder.minScore(this.minScore == null ? Float.MIN_VALUE : this.minScore);
136137
}
137138
if (searchSourceBuilder.size() + searchSourceBuilder.from() > rankDocResults.length) {
138139
searchSourceBuilder.size(Math.max(0, rankDocResults.length - searchSourceBuilder.from()));

server/src/test/java/org/elasticsearch/search/retriever/RankDocsRetrieverBuilderTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ private List<QueryBuilder> preFilters(QueryRewriteContext queryRewriteContext) t
9797
}
9898

9999
private RankDocsRetrieverBuilder createRandomRankDocsRetrieverBuilder(QueryRewriteContext queryRewriteContext) throws IOException {
100-
return new RankDocsRetrieverBuilder(randomIntBetween(1, 100), innerRetrievers(queryRewriteContext), rankDocsSupplier());
100+
return new RankDocsRetrieverBuilder(randomIntBetween(1, 100), innerRetrievers(queryRewriteContext), rankDocsSupplier(), null);
101101
}
102102

103103
public void testExtractToSearchSourceBuilder() throws IOException {

x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/InferenceFeatures.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ public Set<NodeFeature> getTestFeatures() {
5252
SemanticInferenceMetadataFieldsMapper.EXPLICIT_NULL_FIXES,
5353
SEMANTIC_KNN_VECTOR_QUERY_REWRITE_INTERCEPTION_SUPPORTED,
5454
TextSimilarityRankRetrieverBuilder.TEXT_SIMILARITY_RERANKER_ALIAS_HANDLING_FIX,
55+
TextSimilarityRankRetrieverBuilder.TEXT_SIMILARITY_RERANKER_MINSCORE_FIX,
5556
SemanticInferenceMetadataFieldsMapper.INFERENCE_METADATA_FIELDS_ENABLED_BY_DEFAULT,
5657
SEMANTIC_TEXT_HIGHLIGHTER_DEFAULT,
5758
SEMANTIC_KNN_FILTER_FIX,

x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/rank/textsimilarity/TextSimilarityRankRetrieverBuilder.java

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import org.elasticsearch.xcontent.XContentParser;
2424

2525
import java.io.IOException;
26+
import java.util.ArrayList;
2627
import java.util.List;
2728
import java.util.Objects;
2829

@@ -39,6 +40,7 @@ public class TextSimilarityRankRetrieverBuilder extends CompoundRetrieverBuilder
3940
public static final NodeFeature TEXT_SIMILARITY_RERANKER_ALIAS_HANDLING_FIX = new NodeFeature(
4041
"text_similarity_reranker_alias_handling_fix"
4142
);
43+
public static final NodeFeature TEXT_SIMILARITY_RERANKER_MINSCORE_FIX = new NodeFeature("text_similarity_reranker_minscore_fix");
4244

4345
public static final ParseField RETRIEVER_FIELD = new ParseField("retriever");
4446
public static final ParseField INFERENCE_ID_FIELD = new ParseField("inference_id");
@@ -157,23 +159,21 @@ protected TextSimilarityRankRetrieverBuilder clone(
157159
protected RankDoc[] combineInnerRetrieverResults(List<ScoreDoc[]> rankResults, boolean explain) {
158160
assert rankResults.size() == 1;
159161
ScoreDoc[] scoreDocs = rankResults.getFirst();
160-
TextSimilarityRankDoc[] textSimilarityRankDocs = new TextSimilarityRankDoc[scoreDocs.length];
162+
List<TextSimilarityRankDoc> filteredDocs = new ArrayList<>();
163+
// Filtering by min_score must be done here, after reranking.
164+
// Applying min_score in the child retriever could prematurely exclude documents that would receive high scores from the reranker.
161165
for (int i = 0; i < scoreDocs.length; i++) {
162166
ScoreDoc scoreDoc = scoreDocs[i];
163167
assert scoreDoc.score >= 0;
164-
if (explain) {
165-
textSimilarityRankDocs[i] = new TextSimilarityRankDoc(
166-
scoreDoc.doc,
167-
scoreDoc.score,
168-
scoreDoc.shardIndex,
169-
inferenceId,
170-
field
171-
);
172-
} else {
173-
textSimilarityRankDocs[i] = new TextSimilarityRankDoc(scoreDoc.doc, scoreDoc.score, scoreDoc.shardIndex);
168+
if (minScore == null || scoreDoc.score >= minScore) {
169+
if (explain) {
170+
filteredDocs.add(new TextSimilarityRankDoc(scoreDoc.doc, scoreDoc.score, scoreDoc.shardIndex, inferenceId, field));
171+
} else {
172+
filteredDocs.add(new TextSimilarityRankDoc(scoreDoc.doc, scoreDoc.score, scoreDoc.shardIndex));
173+
}
174174
}
175175
}
176-
return textSimilarityRankDocs;
176+
return filteredDocs.toArray(new TextSimilarityRankDoc[0]);
177177
}
178178

179179
@Override

x-pack/plugin/inference/src/yamlRestTest/resources/rest-api-spec/test/inference/70_text_similarity_rank_retriever.yml

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -379,3 +379,111 @@ setup:
379379
- match: { hits.total.value: 1 }
380380
- length: { hits.hits: 1 }
381381
- match: { hits.hits.0._id: "doc_1" }
382+
383+
---
384+
"Text similarity reranker respects min_score":
385+
386+
- requires:
387+
cluster_features: "text_similarity_reranker_minscore_fix"
388+
reason: test min score functionality
389+
390+
- do:
391+
index:
392+
index: test-index
393+
id: doc_2
394+
body:
395+
text: "The phases of the Moon come from the position of the Moon relative to the Earth and Sun."
396+
topic: [ "science" ]
397+
subtopic: [ "astronomy" ]
398+
inference_text_field: "10"
399+
refresh: true
400+
401+
- do:
402+
search:
403+
index: test-index
404+
body:
405+
track_total_hits: true
406+
fields: [ "text", "topic" ]
407+
retriever:
408+
text_similarity_reranker:
409+
retriever:
410+
standard:
411+
query:
412+
bool:
413+
should:
414+
- constant_score:
415+
filter:
416+
term: { subtopic: "technology" }
417+
boost: 10
418+
- constant_score:
419+
filter:
420+
term: { subtopic: "astronomy" }
421+
boost: 1
422+
rank_window_size: 10
423+
inference_id: my-rerank-model
424+
inference_text: "How often does the moon hide the sun?"
425+
field: inference_text_field
426+
min_score: 10
427+
size: 10
428+
429+
- match: { hits.total.value: 1 }
430+
- length: { hits.hits: 1 }
431+
- match: { hits.hits.0._id: "doc_2" }
432+
433+
---
434+
"Text similarity reranker with min_score zero includes all docs":
435+
436+
- requires:
437+
cluster_features: "text_similarity_reranker_minscore_fix"
438+
reason: test min score functionality
439+
440+
- do:
441+
search:
442+
index: test-index
443+
body:
444+
track_total_hits: true
445+
fields: [ "text", "topic" ]
446+
retriever:
447+
text_similarity_reranker:
448+
retriever:
449+
standard:
450+
query:
451+
match_all: {}
452+
rank_window_size: 10
453+
inference_id: my-rerank-model
454+
inference_text: "How often does the moon hide the sun?"
455+
field: inference_text_field
456+
min_score: 0
457+
size: 10
458+
459+
- match: { hits.total.value: 3 }
460+
- length: { hits.hits: 3 }
461+
462+
---
463+
"Text similarity reranker with high min_score excludes all docs":
464+
465+
- requires:
466+
cluster_features: "text_similarity_reranker_minscore_fix"
467+
reason: test min score functionality
468+
469+
- do:
470+
search:
471+
index: test-index
472+
body:
473+
track_total_hits: true
474+
fields: [ "text", "topic" ]
475+
retriever:
476+
text_similarity_reranker:
477+
retriever:
478+
standard:
479+
query:
480+
match_all: {}
481+
rank_window_size: 10
482+
inference_id: my-rerank-model
483+
inference_text: "How often does the moon hide the sun?"
484+
field: inference_text_field
485+
min_score: 1000
486+
size: 10
487+
488+
- match: { hits.total.value: 0 }
489+
- length: { hits.hits: 0 }

0 commit comments

Comments
 (0)