Fix copy_to functionality with vector fields.#3162
Fix copy_to functionality with vector fields.#3162krocky-cooky wants to merge 2 commits intoopensearch-project:mainfrom
Conversation
krocky-cooky
requested review from
0ctopus13prime,
VijayanB,
Vikasht34,
heemin32,
jmazanec15,
junqiu-lei,
luyuncheng,
martin-gaievski,
naveentatikonda,
navneet1v,
ryanbogan,
shatejas and
vamshin
as code owners
krocky-cooky
force-pushed
the
fix/vector-copyto-2636
branch
2 times, most recently
from
976e010 to
dbdf7f6
Compare
Signed-off-by: Takuo Kuroki <kurotaku9679.sub@gmail.com>
krocky-cooky
force-pushed
the
fix/vector-copyto-2636
branch
from
dbdf7f6 to
6579880
Compare
kotwanikunal
left a comment
kotwanikunal
left a comment
There was a problem hiding this comment.
@krocky-cooky - Thanks for raising the PR for testing the copy_to functionality. Before we can close out the linked issue - we need some additional tests with DerivedSource enabled as well as non happy path test cases.
| String targetField1 = "target_vector_1"; | ||
| String targetField2 = "target_vector_2"; | ||
|
|
||
| String mapping = XContentFactory.jsonBuilder() |
There was a problem hiding this comment.
There is a lot of duplication with the mapping creation logic. Can you please pull it out into a helper method or extend an existing one?
| .endObject() | ||
| .startObject(targetField1) | ||
| .field("type", "knn_vector") | ||
| .field("dimension", 2) |
There was a problem hiding this comment.
Can you add in a case where the dimensions across the mappings mismatch?
| assertEquals(1, results2.size()); | ||
| assertEquals("1", results2.get(0).getDocId()); | ||
|
|
||
| deleteKNNIndex(indexName); |
There was a problem hiding this comment.
Please wrap the delete behind a finally block - that should avoid any flakiness.
| } | ||
|
|
||
| @SneakyThrows | ||
| public void testCopyTo_whenSearchOnTargetField_thenSuccess() { |
There was a problem hiding this comment.
Can you also please add in a new test within DerivedSourceIT with the copy to functionality? That path is crucial and we should validate copyTo works with derived source enabled before closing out the issue
2 participants
Description
Previously, adding the copy_to attribute to a vector field would cause an error during indexing. The root cause of this issue is the same as the cause of geo_point issue(opensearch-project/OpenSearch#20540) and has been resolved by opensearch-project/OpenSearch#20542. This PR includes integration tests that verify the copy_to attribute works correctly on vector fields.
Related Issues
Resolves #2636
Check List
--signoff.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.