Fix copy_to functionality with vector fields.

[krocky-cooky]

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

• New functionality includes testing.
• New functionality has been documented.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff.
• Public documentation issue/PR created.

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.

[kotwanikunal]

@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.

[kotwanikunal]

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?

[kotwanikunal]

Can you add in a case where the dimensions across the mappings mismatch?

[krocky-cooky]

Added non happy path test cases!

[kotwanikunal]

Please wrap the delete behind a finally block - that should avoid any flakiness.

[krocky-cooky]

Since ODFERestTestCase already has index cleanup logic in its @after method (wipeAllODFEIndices)
, I believe flakiness should be mitigated. That said, would you still prefer adding try-finally
blocks for clarity? My concern is that it would increase nesting depth in the test methods.

[kotwanikunal]

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

[krocky-cooky]

Added!

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.87%. Comparing base (b8982af) to head (819a512).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3162      +/-   ##
============================================
- Coverage     83.89%   83.87%   -0.02%     
  Complexity     4430     4430              
============================================
  Files           456      456              
  Lines         15981    15981              
  Branches       2101     2101              
============================================
- Hits          13407    13404       -3     
- Misses         1777     1779       +2     
- Partials        797      798       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit cb9ff23.

Path	Line	Severity	Description
jni/external/faiss	1	high	Git submodule (faiss) commit pointer updated from 5616caad4c to 7865ac1dc5. Submodule changes point to arbitrary external commits and cannot be verified for authenticity without manual review of the target commit in the upstream repository.
jni/external/nmslib	1	high	Git submodule (nmslib) commit pointer updated from a2d6624e13 to c9662e44c0. Submodule changes point to arbitrary external commits and cannot be verified for authenticity without manual review of the target commit in the upstream repository.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 2 | Medium: 0 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 812e48b)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 812e48b

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Make dimension-mismatch assertion more robust The source field has dimension 2 and the doc provides a 2-dim vector, so indexing into the source succeeds; the failure is expected only when copying into the 3-dim target. The error message "Vector dimension mismatch" may not match the actual error thrown by the mapper for the target field. Consider asserting a more generic substring (e.g., "dimension") or verifying the exact exception message produced by the copy_to target validation to avoid brittle/false-passing assertions. src/test/java/org/opensearch/knn/index/OpenSearchIT.java [1983-1989] 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")); +String errorBody = EntityUtils.toString(ex.getResponse().getEntity()); +assertThat(errorBody.toLowerCase(), containsString("dimension")); deleteKNNIndex(indexName); Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about brittle error message assertions, but the exact message may in fact be correct given test author intent. Loosening the assertion is a minor robustness improvement.	Low
	Loosen exact error-message assertion Same concern as in OpenSearchIT: the exact error string "Vector dimension mismatch" may not be produced when copy_to routes to a target field with a different dimension. Assert against a more general substring like "dimension" to reduce coupling to internal error messages. src/test/java/org/opensearch/knn/integ/DerivedSourceIT.java [511-517] 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")); +String body = EntityUtils.toString(ex.getResponse().getEntity()); +assertThat(body.toLowerCase(), containsString("dimension")); deleteKNNIndex(indexName); Suggestion importance[1-10]: 4 __ Why: Similar minor robustness improvement to the assertion. Not critical but could reduce test brittleness if the exact error text changes.	Low

---

Previous suggestions

Suggestions up to commit 819a512

Category	Suggestion	Impact
General	Also assert target field source parity The test compares only the sourceField between derived and baseline indices, but since copy_to is used, the targetField should also be validated to ensure the derived source correctly excludes/handles the copied vector field. Otherwise, a regression where the target field is inadvertently persisted in _source on the derived side would go undetected. src/test/java/org/opensearch/knn/integ/DerivedSourceIT.java [466-471] Float[][] vectors = { { 1.0f, 2.0f, 3.0f }, { 10.0f, 20.0f, 30.0f }, { 5.0f, 5.0f, 5.0f } }; for (int i = 0; i < vectors.length; i++) { String docId = String.valueOf(i + 1); addKnnDoc(derivedIndex, docId, sourceField, vectors[i]); addKnnDoc(baselineIndex, docId, sourceField, vectors[i]); } refreshIndex(derivedIndex); refreshIndex(baselineIndex); // Verify _source matches between derived and non-derived indices for (int i = 0; i < vectors.length; i++) { String docId = String.valueOf(i + 1); Map<String, Object> derivedDoc = getKnnDoc(derivedIndex, docId); Map<String, Object> baselineDoc = getKnnDoc(baselineIndex, docId); assertEquals("Source field _source mismatch for doc " + docId, baselineDoc.get(sourceField), derivedDoc.get(sourceField)); + assertEquals("Target field _source mismatch for doc " + docId, baselineDoc.get(targetField), derivedDoc.get(targetField)); } Suggestion importance[1-10]: 6 __ Why: Adding an assertion for the targetField in _source parity between derived and baseline indices strengthens the test coverage for copy_to behavior, catching regressions where the copied field could be inadvertently persisted differently.	Low
	Assert ordering of all returned results After forceMergeKnnIndex, only the top-1 result is validated by ID while k=2 is requested. To make the test more meaningful and catch ordering issues, also assert the second nearest neighbor's ID (should be "2" for query near [1,1]). src/test/java/org/opensearch/knn/index/OpenSearchIT.java [1924-1927] float[] queryVector = { 1.0f, 1.0f }; List<KNNResult> results = getResults(indexName, targetField, queryVector, 2); assertEquals(2, results.size()); assertEquals("1", results.get(0).getDocId()); +assertEquals("2", results.get(1).getDocId()); Suggestion importance[1-10]: 4 __ Why: Asserting the second neighbor's ID improves test rigor since k=2 is requested, but this is a minor test-quality improvement rather than a critical fix.	Low

Suggestions up to commit 020134b

Category	Suggestion	Impact
General	Validate copyTo parameter type The copyTo parameter accepts Object type but is used directly in field() method without validation. If an incompatible type is passed, it could cause runtime errors. Consider validating the type or using a more specific parameter type like String or String[]. src/test/java/org/opensearch/knn/index/OpenSearchIT.java [1980-2000] 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) { + if (!(copyTo instanceof String || copyTo instanceof String[])) { + throw new IllegalArgumentException("copyTo must be String or String[]"); + } builder.field("copy_to", copyTo); } return builder.endObject(); } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that copyTo parameter accepts Object type and could benefit from validation. However, looking at the PR usage, the method is called with String, String[], and null values, which are all valid. The XContentBuilder.field() method likely handles these types appropriately. While adding validation would improve robustness, this is a test utility method with controlled usage patterns, making the impact moderate rather than critical.	Low

Suggestions up to commit be9f72e

Category	Suggestion	Impact
General	Use specific type for parameter The copyTo parameter accepts Object type but is used directly in field("copy_to", copyTo). This could lead to unexpected behavior if non-String/array types are passed. Consider validating the type or using a more specific parameter type like String or String[] to ensure type safety. src/test/java/org/opensearch/knn/index/OpenSearchIT.java [1980-2000] private static XContentBuilder buildKnnVectorFieldMapping( XContentBuilder builder, String fieldName, int dimension, SpaceType spaceType, KNNEngine engine, - Object copyTo + String 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(); } Suggestion importance[1-10]: 3 __ Why: The suggestion to change copyTo parameter from Object to String is partially valid but overlooks that the method is called with both String (line 1824) and String[] (line 1790). The Object type is intentionally used to support both single and multiple target fields. While type validation could improve safety, the suggested change would break existing functionality.	Low

[github-actions]

Persistent review updated to latest commit 020134b

[github-actions]

Persistent review updated to latest commit 819a512

[github-actions]

Persistent review updated to latest commit 812e48b