-
Notifications
You must be signed in to change notification settings - Fork 126
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
Fix lucene codec after lucene version bumped to 9.12 #2195
Conversation
bcdaeec
to
614163a
Compare
Seems like the min distribution is still not updated with the lucene version. Hence the builds are failing. Will check on Jenkins. Ref: https://build.ci.opensearch.org/view/all/job/distribution-build-opensearch/ |
Conversation happening with the build team on this thread: https://opensearch.slack.com/archives/C04UTNM338A/p1728341414662579 |
On further checking and talking to build team we found out that lucene upgrade was merged in opensearch in last 12hrs, which updated the maven repo(which happens with every merge in main branch) but the min distribution runs once in 24hrs hence min distribution is not updated. Thanks @gaiksaya for helping here. A new build is triggered ref: https://build.ci.opensearch.org/blue/organizations/jenkins/publish-opensearch-min-snapshots/detail/publish-opensearch-min-snapshots/1818/pipeline/ Once it is completed I will re-run the GH actions. |
614163a
to
5c9e0fd
Compare
On doing deep-dive on the failed ITs I found out that lucene has changed the way they were using the FlatFieldVectorWriter. This will require more changes in the code to ensure that tests are passing since the changes are in the indexing path. I am working on the fix for this. Will try to raise a PR by today. Ref: apache/lucene#13538 Earlier the Lucene99FlatVectorsWriter.FieldWriter we calling the KNNFieldWriter as a delegate. Now it is not calling anymore, hence we need to call Lucene99FlatVectorsWriter.FieldWriter.addValue from out NativeEngineFieldsVectorWriter.addValue |
5c9e0fd
to
a9ae2a4
Compare
src/main/java/org/opensearch/knn/index/codec/KNN990Codec/NativeEngines990KnnVectorsWriter.java
Outdated
Show resolved
Hide resolved
a9ae2a4
to
6784796
Compare
src/main/java/org/opensearch/knn/index/codec/params/KNNScalarQuantizedVectorsFormatParams.java
Outdated
Show resolved
Hide resolved
6784796
to
bf2234a
Compare
bf2234a
to
4f23631
Compare
4f23631
to
8baeaba
Compare
src/main/java/org/opensearch/knn/index/codec/KNNCodecVersion.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Navneet Verma <[email protected]>
8baeaba
to
c6369dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall NativeEngineWriter perspective it looks good, for isCompress flag please rely on approval from other maintainers.
- We can look at if there is a way to leverage FlatVectorFieldsWriter to get the vectors maybe as a follow up for this. I know NativeEngineWriter uses Map and FlatVectorFieldsWriter uses List but might be worth it if we can leverage to reduce ram usage similar to lucene.
- I see use of any() in unit tests, those matchers don't make for tight tests. Try to verify with exact values if possible
the use of any is added in where we are completely mocking the NativeEngineFieldVectorsWriter flush and merge tests, for other places I have removed it already.
Yes, this is a good suggestion. I have it my mind but problem is if I do it right now the scope of the PR will be huge. Already it had items which came as a part of interface changes. |
Signed-off-by: Navneet Verma <[email protected]> (cherry picked from commit 3322912)
Adding backport label as core has backported the lucene upgrade PR to 2.x branch: opensearch-project/OpenSearch#16211 |
…2196) * Fix lucene codec after lucene version bumped to 9.12 (#2195) Signed-off-by: Navneet Verma <[email protected]> (cherry picked from commit 3322912) * Update CHANGELOG.md Signed-off-by: Navneet Verma <[email protected]> --------- Signed-off-by: Navneet Verma <[email protected]> Co-authored-by: Navneet Verma <[email protected]>
Created a GH issue for the fix: #2207 |
Description
Fix lucene codec after lucene version bumped to 9.12
Currently the version bump has happened only for main branch hence we are not doing any backport here.2.x port done for core: opensearch-project/OpenSearch#16211
This change includes:
Related Issues
Resolves #2193
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.