Skip to content
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

Backport/backport 208 to 2.x #210

Closed

Conversation

zane-neo
Copy link
Collaborator

@zane-neo zane-neo commented Jul 11, 2023

Description

This PR fixed the issue when updating a document with knn_vector field, the update fails because the vector dimension is not matching the required dimension. This PR is a backport PR of: #208

Issues Resolved

#207

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed as per the DCO using --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.

…rch-project#208)

* Fix update document with knnn_vector size not matching issue

Signed-off-by: zane-neo <[email protected]>

* Add apache common lang3 back to fix  PR gradle build failure

Signed-off-by: zane-neo <[email protected]>

* Fix PR check failure

Signed-off-by: zane-neo <[email protected]>

* Fix PR check failure

Signed-off-by: zane-neo <[email protected]>

* Fix PR check jar hell failure

Signed-off-by: zane-neo <[email protected]>

* Add apache common lang back to fix the PR check

Signed-off-by: zane-neo <[email protected]>

* Fix register model group failure in IT

Signed-off-by: zane-neo <[email protected]>

* format code

Signed-off-by: zane-neo <[email protected]>

* Rename the method appendVectorFieldsToDocument to setVectorFieldsToDocument

Signed-off-by: zane-neo <[email protected]>

* format code

Signed-off-by: zane-neo <[email protected]>

---------

Signed-off-by: zane-neo <[email protected]>
(cherry picked from commit 9599371)
Signed-off-by: zane-neo <[email protected]>
@@ -36,6 +36,8 @@ public class MLCommonsClientAccessor {
private static final List<String> TARGET_RESPONSE_FILTERS = List.of("sentence_embedding");
private final MachineLearningNodeClient mlClient;

private static final int MAX_RETRY = 3;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this is added?

Copy link
Collaborator Author

@zane-neo zane-neo Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, I followed the guidance of backport on github, this could be missing removal in previous backport PRs. I've removed this.

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #210 (9eb517e) into 2.x (5228fc7) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##                2.x     #210   +/-   ##
=========================================
  Coverage     89.55%   89.55%           
  Complexity      103      103           
=========================================
  Files             7        7           
  Lines           316      316           
  Branches         52       52           
=========================================
  Hits            283      283           
  Misses           16       16           
  Partials         17       17           
Impacted Files Coverage Δ
...neuralsearch/processor/TextEmbeddingProcessor.java 92.85% <100.00%> (ø)

Copy link
Member

@martin-gaievski martin-gaievski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rechecked PRs content, isn't it exactly same as #208? #208 is also merged to 2.x, so this PR (210) looks like a duplicate.
@zane-neo can you please check that this PR is actually needed, @ylwu please help in checking this today

@navneet1v
Copy link
Collaborator

@zane-neo we should close this PR. The backport has already happened for 2.x branch

@navneet1v navneet1v closed this Jul 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants