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

Fixed a bug that does not get a space type from index setting when it is empty for compatibility. #2309

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

0ctopus13prime
Copy link
Contributor

Description

Issue : #2296

This PR is to fix the mapper to acquire the space type value from index setting for legacy case.
It is deprecated, but it is still available to set the space type at index level. But the current logic picks the default L2 metric when it failed to find the one from top level and method causing the score discrepancy. Even cosine similarity metric was configured at index level, it is not being considered in 2.17 because of the bug.
Hence, this PR is to fix the bug to be backward compatibility.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

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.

Copy link
Collaborator

@navneet1v navneet1v left a comment

Choose a reason for hiding this comment

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

Please add ITs for this change. so that we don't get this bug in future.

@0ctopus13prime
Copy link
Contributor Author

@heemin32
Hi Heemin, could you validate the change in testPainlessScript_whenNonBinary_thenException?
It was using script against to a disabled KNN index.

@heemin32
Copy link
Collaborator

heemin32 commented Dec 5, 2024

@heemin32 Hi Heemin, could you validate the change in testPainlessScript_whenNonBinary_thenException? It was using script against to a disabled KNN index.

painless script is supposed to work regardless KNN index is enabled or not.

@0ctopus13prime
Copy link
Contributor Author

Seems like several IT cases are failing.
Let me change SpaceTypeResolver.INSTANCE.resolveSpaceType to make it fallback to index settings.
With this change, legacy logic will remain as compatible.

@0ctopus13prime
Copy link
Contributor Author

Only Difference

Affected changes by this would be for 2.17+ version to visit index settings to get space_type when method + top level don't have space_type.
Baseline is picking up the default space_type based on vector data type for such case. This will be changed to visit index settings to see whether it has space_type. After that it will fallback to the default space_type based on its vector data type.

Table Columns Explanation

  • Actual Space Type : The final space_type engine would get in the end.
  • Method : space_type at method blob of mapping.
  • Top Level : space_type at top level mapping blob.
  • Index : space_type at index level.

For example, below table describes that the engine got X as space_type when X was configured in method blob.

Version Actual Space Type Method Top Level Index
2.Y (Y >= 17) X null X Dont Care

ASIS

Version Actual Space Type Method Top Level Index
2.Z (Z < 17) cos Dont Care Dont Care cos
2.Y (Y >= 17) X X Dont Care Dont Care
2.Y (Y >= 17) X null X Dont Care
2.Y (Y >= 17) default based on data type null null X

TOBE

Version Actual Space Type Method Top Level Index
2.Z (Z < 17) cos Dont Care Dont Care cos
2.Y (Y >= 17) X X Dont Care Dont Care
2.Y (Y >= 17) X null X Dont Care
2.Y (Y >= 17) X null null X
2.Y (Y >= 17) default based on data type null null null

@0ctopus13prime
Copy link
Contributor Author

0ctopus13prime commented Dec 5, 2024

BWC is failing:
will continue investigating the root cause.

Cannot set modelId or method parameters when index.knn setting is false

@0ctopus13prime 0ctopus13prime force-pushed the space_type_bug_fix branch 2 times, most recently from f189275 to 71d6f73 Compare December 5, 2024 23:09
@0ctopus13prime
Copy link
Contributor Author

BWC is failing :

»  Likely root cause: org.opensearch.common.ValidationException: Validation Failed: 1: "hnsw" with "faiss" configuration does not support space type: "linf".;

… is empty for compatibility.

Signed-off-by: Dooyong Kim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants