-
Notifications
You must be signed in to change notification settings - Fork 243
Fix copy_to functionality with vector fields. #3162
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ | |
| import org.opensearch.knn.KNNCompressionRestTestCase; | ||
| import org.opensearch.knn.KNNResult; | ||
| import org.apache.hc.core5.http.io.entity.EntityUtils; | ||
| import org.opensearch.Version; | ||
| import org.opensearch.client.Request; | ||
| import org.opensearch.client.Response; | ||
| import org.opensearch.client.ResponseException; | ||
|
|
@@ -1786,6 +1787,230 @@ public void testKNNSearchWithProfilerEnabled_Radial() throws Exception { | |
| deleteKNNIndex(INDEX_NAME); | ||
| } | ||
|
|
||
| @SneakyThrows | ||
| public void testCopyTo_whenSearchOnTargetField_thenSuccess() { | ||
| // copy_to for knn_vector requires OpenSearch 3.6.0+ | ||
| if (Version.CURRENT.before(Version.V_3_6_0)) { | ||
| return; | ||
| } | ||
| String indexName = "test_copy_to_search"; | ||
| String sourceField = "source_vector"; | ||
| String targetField1 = "target_vector_1"; | ||
| String targetField2 = "target_vector_2"; | ||
|
|
||
| XContentBuilder builder = XContentFactory.jsonBuilder().startObject().startObject("properties"); | ||
| buildKnnVectorFieldMapping(builder, sourceField, 2, SpaceType.L2, KNNEngine.FAISS, new String[] { targetField1, targetField2 }); | ||
| buildKnnVectorFieldMapping(builder, targetField1, 2, SpaceType.L2, KNNEngine.FAISS, null); | ||
| buildKnnVectorFieldMapping(builder, targetField2, 2, SpaceType.L2, KNNEngine.LUCENE, null); | ||
| String mapping = builder.endObject().endObject().toString(); | ||
|
|
||
| createKnnIndex(indexName, mapping); | ||
|
|
||
| addKnnDoc(indexName, "1", sourceField, new Float[] { 1.0f, 1.0f }); | ||
| addKnnDoc(indexName, "2", sourceField, new Float[] { 10.0f, 10.0f }); | ||
| refreshAllIndices(); | ||
|
|
||
| float[] queryVector = { 1.0f, 1.0f }; | ||
| List<KNNResult> results1 = getResults(indexName, targetField1, queryVector, 1); | ||
| assertEquals(1, results1.size()); | ||
| assertEquals("1", results1.get(0).getDocId()); | ||
|
|
||
| List<KNNResult> results2 = getResults(indexName, targetField2, queryVector, 1); | ||
| assertEquals(1, results2.size()); | ||
| assertEquals("1", results2.get(0).getDocId()); | ||
|
|
||
| deleteKNNIndex(indexName); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please wrap the delete behind a finally block - that should avoid any flakiness.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since ODFERestTestCase already has index cleanup logic in its @after method (wipeAllODFEIndices) |
||
| } | ||
|
|
||
| @SneakyThrows | ||
| public void testCopyTo_whenDocUpdated_thenTargetFieldReflectsUpdate() { | ||
| // copy_to for knn_vector requires OpenSearch 3.6.0+ | ||
| if (Version.CURRENT.before(Version.V_3_6_0)) { | ||
| return; | ||
| } | ||
| String indexName = "test_copy_to_update"; | ||
| String sourceField = "source_vector"; | ||
| String targetField = "target_vector"; | ||
|
|
||
| XContentBuilder builder = XContentFactory.jsonBuilder().startObject().startObject("properties"); | ||
| buildKnnVectorFieldMapping(builder, sourceField, 2, SpaceType.L2, KNNEngine.FAISS, targetField); | ||
| buildKnnVectorFieldMapping(builder, targetField, 2, SpaceType.L2, KNNEngine.FAISS, null); | ||
| String mapping = builder.endObject().endObject().toString(); | ||
|
|
||
| createKnnIndex(indexName, mapping); | ||
|
|
||
| // Insert doc near [10, 10] | ||
| addKnnDoc(indexName, "1", sourceField, new Float[] { 10.0f, 10.0f }); | ||
| refreshAllIndices(); | ||
|
|
||
| // Verify target field finds doc 1 near [10, 10] | ||
| float[] queryNearTen = { 10.0f, 10.0f }; | ||
| List<KNNResult> results = getResults(indexName, targetField, queryNearTen, 1); | ||
| assertEquals(1, results.size()); | ||
| assertEquals("1", results.get(0).getDocId()); | ||
|
|
||
| // Update doc to be near [1, 1] | ||
| updateKnnDoc(indexName, "1", sourceField, new Float[] { 1.0f, 1.0f }); | ||
| refreshAllIndices(); | ||
|
|
||
| // Verify target field reflects the update | ||
| float[] queryNearOne = { 1.0f, 1.0f }; | ||
| results = getResults(indexName, targetField, queryNearOne, 1); | ||
| assertEquals(1, results.size()); | ||
| assertEquals("1", results.get(0).getDocId()); | ||
|
|
||
| deleteKNNIndex(indexName); | ||
| } | ||
|
|
||
| @SneakyThrows | ||
| public void testCopyTo_whenDocDeleted_thenTargetFieldReflectsDeletion() { | ||
| // copy_to for knn_vector requires OpenSearch 3.6.0+ | ||
| if (Version.CURRENT.before(Version.V_3_6_0)) { | ||
| return; | ||
| } | ||
| String indexName = "test_copy_to_delete"; | ||
| String sourceField = "source_vector"; | ||
| String targetField = "target_vector"; | ||
|
|
||
| XContentBuilder builder = XContentFactory.jsonBuilder().startObject().startObject("properties"); | ||
| buildKnnVectorFieldMapping(builder, sourceField, 2, SpaceType.L2, KNNEngine.FAISS, targetField); | ||
| buildKnnVectorFieldMapping(builder, targetField, 2, SpaceType.L2, KNNEngine.FAISS, null); | ||
| String mapping = builder.endObject().endObject().toString(); | ||
|
|
||
| createKnnIndex(indexName, mapping); | ||
|
|
||
| addKnnDoc(indexName, "1", sourceField, new Float[] { 1.0f, 1.0f }); | ||
| addKnnDoc(indexName, "2", sourceField, new Float[] { 10.0f, 10.0f }); | ||
| refreshAllIndices(); | ||
| assertEquals(2, getDocCount(indexName)); | ||
|
|
||
| deleteKnnDoc(indexName, "1"); | ||
| refreshAllIndices(); | ||
| assertEquals(1, getDocCount(indexName)); | ||
|
|
||
| // Search on target field should only return doc 2 | ||
| float[] queryVector = { 1.0f, 1.0f }; | ||
| List<KNNResult> results = getResults(indexName, targetField, queryVector, 10); | ||
| assertEquals(1, results.size()); | ||
| assertEquals("2", results.get(0).getDocId()); | ||
|
|
||
| deleteKNNIndex(indexName); | ||
| } | ||
|
|
||
| @SneakyThrows | ||
| public void testCopyTo_whenForceMerge_thenTargetFieldSearchable() { | ||
| // copy_to for knn_vector requires OpenSearch 3.6.0+ | ||
| if (Version.CURRENT.before(Version.V_3_6_0)) { | ||
| return; | ||
| } | ||
| String indexName = "test_copy_to_merge"; | ||
| String sourceField = "source_vector"; | ||
| String targetField = "target_vector"; | ||
|
|
||
| XContentBuilder builder = XContentFactory.jsonBuilder().startObject().startObject("properties"); | ||
| buildKnnVectorFieldMapping(builder, sourceField, 2, SpaceType.L2, KNNEngine.FAISS, targetField); | ||
| buildKnnVectorFieldMapping(builder, targetField, 2, SpaceType.L2, KNNEngine.FAISS, null); | ||
| String mapping = builder.endObject().endObject().toString(); | ||
|
|
||
| createKnnIndex(indexName, mapping); | ||
|
|
||
| addKnnDoc(indexName, "1", sourceField, new Float[] { 1.0f, 1.0f }); | ||
| addKnnDoc(indexName, "2", sourceField, new Float[] { 5.0f, 5.0f }); | ||
| addKnnDoc(indexName, "3", sourceField, new Float[] { 10.0f, 10.0f }); | ||
| refreshAllIndices(); | ||
|
|
||
| forceMergeKnnIndex(indexName); | ||
|
|
||
| float[] queryVector = { 1.0f, 1.0f }; | ||
| List<KNNResult> results = getResults(indexName, targetField, queryVector, 2); | ||
| assertEquals(2, results.size()); | ||
| assertEquals("1", results.get(0).getDocId()); | ||
|
|
||
| deleteKNNIndex(indexName); | ||
| } | ||
|
|
||
| @SneakyThrows | ||
| public void testCopyTo_whenSourceAndTargetBothSearched_thenBothReturnResults() { | ||
| // copy_to for knn_vector requires OpenSearch 3.6.0+ | ||
| if (Version.CURRENT.before(Version.V_3_6_0)) { | ||
| return; | ||
| } | ||
| String indexName = "test_copy_to_both"; | ||
| String sourceField = "source_vector"; | ||
| String targetField = "target_vector"; | ||
|
|
||
| XContentBuilder builder = XContentFactory.jsonBuilder().startObject().startObject("properties"); | ||
| buildKnnVectorFieldMapping(builder, sourceField, 2, SpaceType.L2, KNNEngine.FAISS, targetField); | ||
| buildKnnVectorFieldMapping(builder, targetField, 2, SpaceType.L2, KNNEngine.FAISS, null); | ||
| String mapping = builder.endObject().endObject().toString(); | ||
|
|
||
| createKnnIndex(indexName, mapping); | ||
|
|
||
| addKnnDoc(indexName, "1", sourceField, new Float[] { 1.0f, 1.0f }); | ||
| addKnnDoc(indexName, "2", sourceField, new Float[] { 10.0f, 10.0f }); | ||
| refreshAllIndices(); | ||
|
|
||
| float[] queryVector = { 1.0f, 1.0f }; | ||
|
|
||
| // Both source and target should return the same nearest neighbor | ||
| List<KNNResult> sourceResults = getResults(indexName, sourceField, queryVector, 1); | ||
| List<KNNResult> targetResults = getResults(indexName, targetField, queryVector, 1); | ||
|
|
||
| assertEquals(1, sourceResults.size()); | ||
| assertEquals(1, targetResults.size()); | ||
| assertEquals(sourceResults.get(0).getDocId(), targetResults.get(0).getDocId()); | ||
|
|
||
| deleteKNNIndex(indexName); | ||
| } | ||
|
|
||
| @SneakyThrows | ||
| public void testCopyTo_whenTargetFieldHasDifferentDimension_thenFail() { | ||
| // copy_to for knn_vector requires OpenSearch 3.6.0+ | ||
| if (Version.CURRENT.before(Version.V_3_6_0)) { | ||
| return; | ||
| } | ||
| String indexName = "test_copy_to_dim_mismatch"; | ||
| String sourceField = "source_vector"; | ||
| String targetField = "target_vector"; | ||
|
|
||
| XContentBuilder builder = XContentFactory.jsonBuilder().startObject().startObject("properties"); | ||
| buildKnnVectorFieldMapping(builder, sourceField, 2, SpaceType.L2, KNNEngine.FAISS, targetField); | ||
| buildKnnVectorFieldMapping(builder, targetField, 3, SpaceType.L2, KNNEngine.FAISS, null); | ||
| String mapping = builder.endObject().endObject().toString(); | ||
|
|
||
| createKnnIndex(indexName, mapping); | ||
|
|
||
| ResponseException ex = expectThrows( | ||
| ResponseException.class, | ||
| () -> addKnnDoc(indexName, "1", sourceField, new Float[] { 1.0f, 1.0f }) | ||
| ); | ||
| assertThat(EntityUtils.toString(ex.getResponse().getEntity()), containsString("Vector dimension mismatch")); | ||
|
|
||
| deleteKNNIndex(indexName); | ||
| } | ||
|
|
||
| private static XContentBuilder buildKnnVectorFieldMapping( | ||
| XContentBuilder builder, | ||
| String fieldName, | ||
| int dimension, | ||
| SpaceType spaceType, | ||
| KNNEngine engine, | ||
| Object copyTo | ||
| ) throws IOException { | ||
| builder.startObject(fieldName) | ||
| .field("type", "knn_vector") | ||
| .field("dimension", dimension) | ||
| .startObject("method") | ||
| .field(KNNConstants.NAME, KNNConstants.METHOD_HNSW) | ||
| .field(KNNConstants.METHOD_PARAMETER_SPACE_TYPE, spaceType.getValue()) | ||
| .field(KNNConstants.KNN_ENGINE, engine.getName()) | ||
| .endObject(); | ||
| if (copyTo != null) { | ||
| builder.field("copy_to", copyTo); | ||
| } | ||
| return builder.endObject(); | ||
| } | ||
|
|
||
| private List<KNNResult> getResults(final String indexName, final String fieldName, final float[] vector, final int k) | ||
| throws IOException, ParseException { | ||
| final Response searchResponseField = searchKNNIndex(indexName, new KNNQueryBuilder(fieldName, vector, k), k); | ||
|
|
||
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.
Can you also please add in a new test within
DerivedSourceITwith the copy to functionality? That path is crucial and we should validate copyTo works with derived source enabled before closing out the issueThere 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.
Added!