Fix Synthetic Source Handling for `bit` Type in `dense_vector` Field by Rassyan · Pull Request #114407 · elastic/elasticsearch

Rassyan · 2024-10-09T12:47:04Z

Description:

This PR addresses the issue described in #114402, where the synthetic_source feature does not correctly handle the bit type in dense_vector fields when index is set to false. The root cause of the issue was that the bit type was not properly accounted for, leading to an array that is 8 times the size of the actual dims value of docvalue. This mismatch will causes an array out-of-bounds exception when reconstructing the document.

Changes:

Adjusted the synthetic_source logic to correctly handle the bit type by ensuring the array size accounts for the 8x difference in dimensions.
Added yaml test to cover the bit type scenario in dense_vector fields with index set to false.

Related Issues:

Closes #114402
Introduced in #110059

… `index` is set to `false` (elastic#114402)

Rassyan · 2024-10-09T12:58:19Z

@benwtrent

Could you please take a look at this PR? I would greatly appreciate your review and feedback.
Thank you for your time and consideration.

Best regards,
Rassyan

elasticsearchmachine · 2024-10-09T13:34:39Z

Pinging @elastic/es-search-relevance (Team:Search Relevance)

benwtrent · 2024-10-09T13:40:05Z

@Rassyan thank you for the PR, a couple of things:

We need a new capability added here: org.elasticsearch.rest.action.search.SearchCapabilities so that we can appropriately handle test cases.
Add a changelog file for the bug fix (there are many examples of bug fix change logs).
Update your yaml test to require the capability (there are many examples of other yaml tests where this happens).
Run spotlessApply to fix formatting.

Rassyan · 2024-10-09T14:22:37Z

@benwtrent

Thank you for the feedback. I will start working on the changes immediately.

Update your yaml test to require the capability (there are many examples of other yaml tests where this happens)

Could you please point me to specific examples of yaml tests that demonstrate this?

Thank you again for your guidance.

Best regards,

Rassyan

benwtrent · 2024-10-09T15:05:01Z

@Rassyan search for something like:

  - requires:
      capabilities:

In *.yml files to see it in action.

…c_source`

...-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.vectors/45_knn_search_bit.yml

benwtrent · 2024-10-09T15:37:46Z

buildkite test this please

benwtrent

I think this is looking good. Could we add some more testing lower down in the mapper testing? The yaml test and the fix looks good otherwise.

...-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.vectors/45_knn_search_bit.yml

…cSourceSupport` for element_type bit

server/src/test/java/org/elasticsearch/index/mapper/vectors/DenseVectorFieldMapperTests.java

benwtrent · 2024-10-10T11:47:20Z

buildkite test this please

…identify range check issue

server/src/test/java/org/elasticsearch/index/mapper/vectors/DenseVectorFieldMapperTests.java

benwtrent · 2024-10-10T13:56:40Z

buildkite test this please

benwtrent

Thank you for going through the additional testing. This fix is great :)

benwtrent · 2024-10-10T14:37:24Z

server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java

+            List<String> order = Arrays.stream(params).map(p -> p.name).toList();
+            Iterator<Map.Entry<String, Object>> iterator = fieldNode.entrySet().stream().sorted((e1, e2) -> {
+                int i1 = order.indexOf(e1.getKey());
+                int i2 = order.indexOf(e2.getKey());
+                return Integer.compare(i1, i2);
+            }).iterator();
+            while (iterator.hasNext()) {


I am against this, let's remove to try and find a better fix if we still run into weird parsing ordering failures.

It would be better to update the dense mapper validations than to fundamentally change how all mappers iterate values.

OK, I understand your concerns. I will move the commits related to the mapper bug to a separate PR for further discussion. This PR will remain focused solely on fixing the Synthetic Source issue. Does that sound good to you?

I will move the commits related to the mapper bug to a separate PR for further discussion.

thank you

OK, new PR #114516

benwtrent · 2024-10-10T16:21:14Z

@Rassyan we should be able to update testing here like before without any of the parsing changes. Please revert back to your testing updates, including my updates to synthetic source so we can figure out the testing failures there.

Rassyan · 2024-10-10T16:38:49Z

@Rassyan we should be able to update testing here like before without any of the parsing changes. Please revert back to your testing updates, including my updates to synthetic source so we can figure out the testing failures there.

moved to new PR #114516

…hetic-source

benwtrent · 2024-10-14T17:43:01Z

buildkite test this please

benwtrent · 2024-10-14T17:45:54Z

@Rassyan I have updated the test, I verified it all passes ok, even for 1k dimensions and bits. I think this is good :). I will merge when green.

benwtrent · 2024-10-14T17:48:16Z

buildkite test this please

…lastic#114407) **Description:** This PR addresses the issue described in [elastic#114402](elastic#114402), where the `synthetic_source` feature does not correctly handle the `bit` type in `dense_vector` fields when `index` is set to `false`. The root cause of the issue was that the `bit` type was not properly accounted for, leading to an array that is 8 times the size of the actual `dims` value of docvalue. This mismatch will causes an array out-of-bounds exception when reconstructing the document. **Changes:** - Adjusted the `synthetic_source` logic to correctly handle the `bit` type by ensuring the array size accounts for the 8x difference in dimensions. - Added yaml test to cover the `bit` type scenario in `dense_vector` fields with `index` set to `false`. **Related Issues:** - Closes [elastic#114402](elastic#114402) - Introduced in [elastic#110059](elastic#110059)

elasticsearchmachine · 2024-10-14T19:01:33Z

💔 Backport failed

Status	Branch	Result
✅	8.x
❌	8.15	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 114407

benwtrent · 2024-10-14T19:47:26Z

💚 All backports created successfully

Status	Branch	Result
✅	8.15

Questions ?

Please refer to the Backport tool documentation

…lastic#114407) **Description:** This PR addresses the issue described in [elastic#114402](elastic#114402), where the `synthetic_source` feature does not correctly handle the `bit` type in `dense_vector` fields when `index` is set to `false`. The root cause of the issue was that the `bit` type was not properly accounted for, leading to an array that is 8 times the size of the actual `dims` value of docvalue. This mismatch will causes an array out-of-bounds exception when reconstructing the document. **Changes:** - Adjusted the `synthetic_source` logic to correctly handle the `bit` type by ensuring the array size accounts for the 8x difference in dimensions. - Added yaml test to cover the `bit` type scenario in `dense_vector` fields with `index` set to `false`. **Related Issues:** - Closes [elastic#114402](elastic#114402) - Introduced in [elastic#110059](elastic#110059) (cherry picked from commit 465c65c)

…114407) (#114756) **Description:** This PR addresses the issue described in [#114402](#114402), where the `synthetic_source` feature does not correctly handle the `bit` type in `dense_vector` fields when `index` is set to `false`. The root cause of the issue was that the `bit` type was not properly accounted for, leading to an array that is 8 times the size of the actual `dims` value of docvalue. This mismatch will causes an array out-of-bounds exception when reconstructing the document. **Changes:** - Adjusted the `synthetic_source` logic to correctly handle the `bit` type by ensuring the array size accounts for the 8x difference in dimensions. - Added yaml test to cover the `bit` type scenario in `dense_vector` fields with `index` set to `false`. **Related Issues:** - Closes [#114402](#114402) - Introduced in [#110059](#110059) Co-authored-by: Rassyan <yjkhngds@gmail.com>

…lastic#114407) **Description:** This PR addresses the issue described in [elastic#114402](elastic#114402), where the `synthetic_source` feature does not correctly handle the `bit` type in `dense_vector` fields when `index` is set to `false`. The root cause of the issue was that the `bit` type was not properly accounted for, leading to an array that is 8 times the size of the actual `dims` value of docvalue. This mismatch will causes an array out-of-bounds exception when reconstructing the document. **Changes:** - Adjusted the `synthetic_source` logic to correctly handle the `bit` type by ensuring the array size accounts for the 8x difference in dimensions. - Added yaml test to cover the `bit` type scenario in `dense_vector` fields with `index` set to `false`. **Related Issues:** - Closes [elastic#114402](elastic#114402) - Introduced in [elastic#110059](elastic#110059)

…114407) **Description:** This PR addresses the issue described in [#114402](#114402), where the `synthetic_source` feature does not correctly handle the `bit` type in `dense_vector` fields when `index` is set to `false`. The root cause of the issue was that the `bit` type was not properly accounted for, leading to an array that is 8 times the size of the actual `dims` value of docvalue. This mismatch will causes an array out-of-bounds exception when reconstructing the document. **Changes:** - Adjusted the `synthetic_source` logic to correctly handle the `bit` type by ensuring the array size accounts for the 8x difference in dimensions. - Added yaml test to cover the `bit` type scenario in `dense_vector` fields with `index` set to `false`. **Related Issues:** - Closes [#114402](#114402) - Introduced in [#110059](#110059)

…ld (#114407) (#114759) * Fix Synthetic Source Handling for `bit` Type in `dense_vector` Field (#114407) **Description:** This PR addresses the issue described in [#114402](#114402), where the `synthetic_source` feature does not correctly handle the `bit` type in `dense_vector` fields when `index` is set to `false`. The root cause of the issue was that the `bit` type was not properly accounted for, leading to an array that is 8 times the size of the actual `dims` value of docvalue. This mismatch will causes an array out-of-bounds exception when reconstructing the document. **Changes:** - Adjusted the `synthetic_source` logic to correctly handle the `bit` type by ensuring the array size accounts for the 8x difference in dimensions. - Added yaml test to cover the `bit` type scenario in `dense_vector` fields with `index` set to `false`. **Related Issues:** - Closes [#114402](#114402) - Introduced in [#110059](#110059) (cherry picked from commit 465c65c) * fixing backport of search capabilities * fixing license header * adding capabilities to RestSearchAction * fixing backport * spotless * muting teset for ccs * adding capabilities to the ccs test runner --------- Co-authored-by: Rassyan <yjkhngds@gmail.com>

…lastic#114407) **Description:** This PR addresses the issue described in [elastic#114402](elastic#114402), where the `synthetic_source` feature does not correctly handle the `bit` type in `dense_vector` fields when `index` is set to `false`. The root cause of the issue was that the `bit` type was not properly accounted for, leading to an array that is 8 times the size of the actual `dims` value of docvalue. This mismatch will causes an array out-of-bounds exception when reconstructing the document. **Changes:** - Adjusted the `synthetic_source` logic to correctly handle the `bit` type by ensuring the array size accounts for the 8x difference in dimensions. - Added yaml test to cover the `bit` type scenario in `dense_vector` fields with `index` set to `false`. **Related Issues:** - Closes [elastic#114402](elastic#114402) - Introduced in [elastic#110059](elastic#110059)

Fix synthetic source bug with bit type in dense_vector field when…

7d7778e

… `index` is set to `false` (elastic#114402)

elasticsearchmachine added needs:triage Requires assignment of a team area label external-contributor Pull request authored by a developer outside the Elasticsearch team v9.0.0 labels Oct 9, 2024

benwtrent self-requested a review October 9, 2024 13:33

benwtrent self-assigned this Oct 9, 2024

benwtrent added >bug :Search Relevance/Vectors Vector search v8.16.0 v8.15.4 labels Oct 9, 2024

elasticsearchmachine added Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch and removed needs:triage Requires assignment of a team area label labels Oct 9, 2024

Rassyan added 3 commits October 9, 2024 23:10

Adding changelog for PR elastic#114407

8775603

Fix formatting

3992b94

Update yaml test to require new capability `bit_dense_vector_syntheti…

2473b13

…c_source`

Rassyan commented Oct 9, 2024

View reviewed changes

...-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.vectors/45_knn_search_bit.yml Show resolved Hide resolved

benwtrent reviewed Oct 9, 2024

View reviewed changes

...-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.vectors/45_knn_search_bit.yml Show resolved Hide resolved

Add some coverage to `DenseVectorFieldMapperTests.DenseVectorSyntheti…

bb14889

…cSourceSupport` for element_type bit

Rassyan commented Oct 10, 2024

View reviewed changes

server/src/test/java/org/elasticsearch/index/mapper/vectors/DenseVectorFieldMapperTests.java Outdated Show resolved Hide resolved

Temporarily swapped build order of element_type and dims to help …

18b9b14

…identify range check issue

Rassyan commented Oct 10, 2024

View reviewed changes

server/src/test/java/org/elasticsearch/index/mapper/vectors/DenseVectorFieldMapperTests.java Show resolved Hide resolved

fixing synthetic src testing

572421c

benwtrent approved these changes Oct 10, 2024

View reviewed changes

benwtrent reviewed Oct 10, 2024

View reviewed changes

Rassyan force-pushed the fix-bit-vector-synthetic-source branch from 0f5ca00 to 2473b13 Compare October 10, 2024 15:56

Rassyan mentioned this pull request Oct 11, 2024

Investigate and Address Dense Vector Mapper Parsing Issue #114516

Closed

benwtrent added 2 commits October 14, 2024 13:28

Merge remote-tracking branch 'upstream/main' into fix-bit-vector-synt…

86ac619

…hetic-source

Fixing tests after updating from main

354a810

removing unused import

4c8b2a8

benwtrent added the auto-backport Automatically create backport pull requests when merged label Oct 14, 2024

benwtrent merged commit 465c65c into elastic:main Oct 14, 2024

benwtrent mentioned this pull request Oct 14, 2024

[8.x] Fix Synthetic Source Handling for bit Type in dense_vector Field (#114407) #114756

Merged

elasticsearchmachine added the backport pending label Oct 14, 2024

benwtrent mentioned this pull request Oct 14, 2024

[8.15] Fix Synthetic Source Handling for bit Type in dense_vector Field (#114407) #114759

Merged

benwtrent removed the backport pending label Oct 17, 2024

Conversation

Rassyan commented Oct 9, 2024

Uh oh!

Rassyan commented Oct 9, 2024

Uh oh!

elasticsearchmachine commented Oct 9, 2024

Uh oh!

benwtrent commented Oct 9, 2024

Uh oh!

Rassyan commented Oct 9, 2024

Uh oh!

benwtrent commented Oct 9, 2024

Uh oh!

Uh oh!

benwtrent commented Oct 9, 2024

Uh oh!

benwtrent left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

benwtrent commented Oct 10, 2024

Uh oh!

Uh oh!

benwtrent commented Oct 10, 2024

Uh oh!

benwtrent left a comment

Choose a reason for hiding this comment

Uh oh!

benwtrent Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

Rassyan Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

benwtrent Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

Rassyan Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

benwtrent commented Oct 10, 2024

Uh oh!

Rassyan commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benwtrent commented Oct 14, 2024

Uh oh!

benwtrent commented Oct 14, 2024

Uh oh!

benwtrent commented Oct 14, 2024

Uh oh!

elasticsearchmachine commented Oct 14, 2024

💔 Backport failed

Uh oh!

benwtrent commented Oct 14, 2024

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Rassyan commented Oct 10, 2024 •

edited

Loading