Bump lucene codec to 99 #1383

naveentatikonda · 2024-01-10T22:58:34Z

Description

This PR includes the following changes:

Add new kNN codec that wraps Lucene new codec 9.9 and make it default
Fix import statements for Lucene Codec 95
Fix the arguments mismatch issue with SegmentInfo Constructor
Temporarily Ignore the old lucene codec tests (KNN910Codec, KNN920Codec, KNN940Codec, KNN950Codec). Due to some recent changes in Lucene for SegmentInfo causing these tests to fail which will be fixed later with a separate PR.

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.

Signed-off-by: Naveen Tatikonda <[email protected]>

navneet1v · 2024-01-10T23:16:18Z

@naveentatikonda CI is failing for mac.

codecov · 2024-01-10T23:16:26Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (45282e0) 85.17% compared to head (accf812) 84.87%.

Files	Patch %	Lines
...ec/KNN990Codec/KNN990PerFieldKnnVectorsFormat.java	80.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1383      +/-   ##
============================================
- Coverage     85.17%   84.87%   -0.31%     
+ Complexity     1261     1259       -2     
============================================
  Files           163      165       +2     
  Lines          5114     5138      +24     
  Branches        480      480              
============================================
+ Hits           4356     4361       +5     
- Misses          553      572      +19     
  Partials        205      205

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

naveentatikonda · 2024-01-10T23:19:42Z

@naveentatikonda CI is failing for mac.

Yes, CI is failing on macOS with below error. Not sure what was the exact reason. Guessing if something was wrong while publishing the changes by core to maven because it is a valid class and working fine for other OS.

java.util.ServiceConfigurationError: org.apache.lucene.codecs.Codec: Provider org.opensearch.knn.index.codec.KNN910Codec.KNN910Codec could not be instantiated
»  	at java.base/java.util.ServiceLoader.fail(ServiceLoader.java:586)
»  	at java.base/java.util.ServiceLoader$ProviderImpl.newInstance(ServiceLoader.java:813)
»  	at java.base/java.util.ServiceLoader$ProviderImpl.get(ServiceLoader.java:729)
»  	at java.base/java.util.ServiceLoader$3.next(ServiceLoader.java:1403)
»  	at org.apache.lucene.util.NamedSPILoader.reload(NamedSPILoader.java:68)
»  	at org.apache.lucene.codecs.Codec.reloadCodecs(Codec.java:136)
»  	at org.opensearch.plugins.PluginsService.reloadLuceneSPI(PluginsService.java:753)
»  	at org.opensearch.plugins.PluginsService.loadBundle(PluginsService.java:706)
»  	at org.opensearch.plugins.PluginsService.loadBundles(PluginsService.java:533)
»  	at org.opensearch.plugins.PluginsService.<init>(PluginsService.java:195)
»  	at org.opensearch.node.Node.<init>(Node.java:484)
»  	at org.opensearch.node.Node.<init>(Node.java:411)
»  	at org.opensearch.bootstrap.Bootstrap$5.<init>(Bootstrap.java:242)
»  	at org.opensearch.bootstrap.Bootstrap.setup(Bootstrap.java:242)
»  	at org.opensearch.bootstrap.Bootstrap.init(Bootstrap.java:404)
»  	at org.opensearch.bootstrap.OpenSearch.init(OpenSearch.java:180)
»  	at org.opensearch.bootstrap.OpenSearch.execute(OpenSearch.java:171)
»  	at org.opensearch.cli.EnvironmentAwareCommand.execute(EnvironmentAwareCommand.java:104)
»  	at org.opensearch.cli.Command.mainWithoutErrorHandling(Command.java:138)
»  	at org.opensearch.cli.Command.main(Command.java:101)
»  	at org.opensearch.bootstrap.OpenSearch.main(OpenSearch.java:137)
»  	at org.opensearch.bootstrap.OpenSearch.main(OpenSearch.java:103)
»  Caused by: java.lang.NoClassDefFoundError: org/apache/lucene/backward_codecs/lucene95/Lucene95Codec
»  	at org.opensearch.knn.index.codec.KNN910Codec.KNN910Codec.<clinit>(KNN910Codec.java:20)
»  	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
»  	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
»  	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
»  	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
»  	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480)
»  	at java.base/java.util.ServiceLoader$ProviderImpl$2.run(ServiceLoader.java:797)
»  	at java.base/java.security.AccessController.doPrivileged(AccessController.java:712)
»  	at java.base/java.util.ServiceLoader$ProviderImpl.newInstance(ServiceLoader.java:802)
»  	... 20 more
»  Caused by: java.lang.ClassNotFoundException: org.apache.lucene.backward_codecs.lucene95.Lucene95Codec
»  	at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:445)
»  	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:592)
»  	at java.base/java.net.FactoryURLClassLoader.loadClass(URLClassLoader.java:872)
»  	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
»  	... 29 more

navneet1v · 2024-01-10T23:24:26Z

@naveentatikonda CI is failing for mac.

Yes, CI is failing on macOS with below error. Not sure what was the exact reason. Guessing if something was wrong while publishing the changes by core to maven because it is a valid class and working fine for other OS.

Block (41 lines)

java.util.ServiceConfigurationError: org.apache.lucene.codecs.Codec: Provider org.opensearch.knn.index.codec.KNN910Codec.KNN910Codec could not be instantiated
»  	at java.base/java.util.ServiceLoader.fail(ServiceLoader.java:586)
»  	at java.base/java.util.ServiceLoader$ProviderImpl.newInstance(ServiceLoader.java:813)
»  	at java.base/java.util.ServiceLoader$ProviderImpl.get(ServiceLoader.java:729)
»  	at java.base/java.util.ServiceLoader$3.next(ServiceLoader.java:1403)
»  	at org.apache.lucene.util.NamedSPILoader.reload(NamedSPILoader.java:68)
»  	at org.apache.lucene.codecs.Codec.reloadCodecs(Codec.java:136)
»  	at org.opensearch.plugins.PluginsService.reloadLuceneSPI(PluginsService.java:753)
»  	at org.opensearch.plugins.PluginsService.loadBundle(PluginsService.java:706)
»  	at org.opensearch.plugins.PluginsService.loadBundles(PluginsService.java:533)
»  	at org.opensearch.plugins.PluginsService.<init>(PluginsService.java:195)
»  	at org.opensearch.node.Node.<init>(Node.java:484)
»  	at org.opensearch.node.Node.<init>(Node.java:411)
»  	at org.opensearch.bootstrap.Bootstrap$5.<init>(Bootstrap.java:242)
»  	at org.opensearch.bootstrap.Bootstrap.setup(Bootstrap.java:242)
»  	at org.opensearch.bootstrap.Bootstrap.init(Bootstrap.java:404)
»  	at org.opensearch.bootstrap.OpenSearch.init(OpenSearch.java:180)
»  	at org.opensearch.bootstrap.OpenSearch.execute(OpenSearch.java:171)
»  	at org.opensearch.cli.EnvironmentAwareCommand.execute(EnvironmentAwareCommand.java:104)
»  	at org.opensearch.cli.Command.mainWithoutErrorHandling(Command.java:138)
»  	at org.opensearch.cli.Command.main(Command.java:101)
»  	at org.opensearch.bootstrap.OpenSearch.main(OpenSearch.java:137)
»  	at org.opensearch.bootstrap.OpenSearch.main(OpenSearch.java:103)
»  Caused by: java.lang.NoClassDefFoundError: org/apache/lucene/backward_codecs/lucene95/Lucene95Codec
»  	at org.opensearch.knn.index.codec.KNN910Codec.KNN910Codec.<clinit>(KNN910Codec.java:20)
»  	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
»  	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
»  	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
»  	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
»  	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480)
»  	at java.base/java.util.ServiceLoader$ProviderImpl$2.run(ServiceLoader.java:797)
»  	at java.base/java.security.AccessController.doPrivileged(AccessController.java:712)
»  	at java.base/java.util.ServiceLoader$ProviderImpl.newInstance(ServiceLoader.java:802)
»  	... 20 more
»  Caused by: java.lang.ClassNotFoundException: org.apache.lucene.backward_codecs.lucene95.Lucene95Codec
»  	at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:445)
»  	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:592)
»  	at java.base/java.net.FactoryURLClassLoader.loadClass(URLClassLoader.java:872)
»  	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
»  	... 29 more

can we confirm with core team on this?

martin-gaievski

I think we need to add into this PR capability of providing segmentInfo format. Tests are not critical and can be fixed later, but adding a format should be part of the codec version bump.

src/main/java/org/opensearch/knn/index/codec/KNN990Codec/KNN990Codec.java

navneet1v · 2024-01-11T05:15:56Z

I think we need to add into this PR capability of providing segmentInfo format. Tests are not critical and can be fixed later, but adding a format should be part of the codec version bump.

I don't think so we need that. The older codecs are just used for reading the segment files and not for doing any kind of writes on the segment info file(we can see that BWC are successful here). What the issue happening in unit tests is we are using unit test to create a proper segment with a codec that doesn't support writing to segment files.

I think we should just remove these unit tests as they are not required and keep the unit test for the latest codec.

Nevertheless I synced with @naveentatikonda and asked him to verify this understanding again by doing some kind of manual tests or adding a BWC tests where after upgrade we delete few documents and then do a query.

martin-gaievski · 2024-01-11T17:06:49Z

I think we need to add into this PR capability of providing segmentInfo format. Tests are not critical and can be fixed later, but adding a format should be part of the codec version bump.

I don't think so we need that. The older codecs are just used for reading the segment files and not for doing any kind of writes on the segment info file(we can see that BWC are successful here). What the issue happening in unit tests is we are using unit test to create a proper segment with a codec that doesn't support writing to segment files.

I think we should just remove these unit tests as they are not required and keep the unit test for the latest codec.

Nevertheless I synced with @naveentatikonda and asked him to verify this understanding again by doing some kind of manual tests or adding a BWC tests where after upgrade we delete few documents and then do a query.

Agree for tests for older codecs, we can keep write tests only for latest codec version. I still think we need to add a segment info format to the latest knn codec that is based on lucene 99 codec, similarly as we did for docValues and compound formats in KNNFormatFacade

martin-gaievski · 2024-01-11T17:33:31Z

I think we need to add into this PR capability of providing segmentInfo format. Tests are not critical and can be fixed later, but adding a format should be part of the codec version bump.

I don't think so we need that. The older codecs are just used for reading the segment files and not for doing any kind of writes on the segment info file(we can see that BWC are successful here). What the issue happening in unit tests is we are using unit test to create a proper segment with a codec that doesn't support writing to segment files.
I think we should just remove these unit tests as they are not required and keep the unit test for the latest codec.
Nevertheless I synced with @naveentatikonda and asked him to verify this understanding again by doing some kind of manual tests or adding a BWC tests where after upgrade we delete few documents and then do a query.

Agree for tests for older codecs, we can keep write tests only for latest codec version. I still think we need to add a segment info format to the latest knn codec that is based on lucene 99 codec, similarly as we did for docValues and compound formats in KNNFormatFacade

Why? Is something failing? 🤔

I think we need that extra layer of abstraction to allow usage of delegate codec in classes that may extend for our knn codec class. It was the idea why the format facade class is needed at the first place, maybe things changed now. Otherwise it will use format from default delegate as per parent class implementation in lucene, that's why it's not failing for us.

vibrantvarun · 2024-01-11T18:07:04Z

Here to just try understanding the changes added in the PR

src/test/java/org/opensearch/knn/index/codec/KNN910Codec/KNN910CodecTests.java

navneet1v · 2024-01-11T19:08:50Z

knn codec class

Do we have such classes? I have not seen such classes.

@martin-gaievski

heemin32 · 2024-01-11T19:19:06Z

Agree for tests for older codecs, we can keep write tests only for latest codec version. I still think we need to add a segment info format to the latest knn codec that is based on lucene 99 codec, similarly as we did for docValues and compound formats in KNNFormatFacade

@martin-gaievski For docValues and compound format, we have our custom logic which require us to override get methods for those. For segment info, I don't understand why we need same pattern. What does add a segment info format to the latest knn codec mean?

navneet1v · 2024-01-11T21:17:28Z

@martin-gaievski if a any other plugin is using the KNNCodec then they should also start depending on the latest codec of K-NN. Just like k-NN relies on the latest codec from lucene and we upgrade every time there is an upgrade.

One this which we are not doing is moving the codec to a package which has backward.codec in path which is done by lucene. I think we can take that as a next step.

martin-gaievski · 2024-01-11T21:20:24Z

knn codec class

Do we have such classes? I have not seen such classes.

@martin-gaievski

It was an assumption at the time that facade was added, that users can extend our knn codec or use their own delegate. I haven't seen such classes either.
I think we use the segmentinfo format from default delegate and add it to facade later if needed.

martin-gaievski · 2024-01-11T21:28:58Z

@martin-gaievski if a any other plugin is using the KNNCodec then they should also start depending on the latest codec of K-NN. Just like k-NN relies on the latest codec from lucene and we upgrade every time there is an upgrade.

One this which we are not doing is moving the codec to a package which has backward.codec in path which is done by lucene. I think we can take that as a next step.

While this statement makes sense, especially regarding moving old codecs to a separate package, my point was different. I was referring to an option of passing custom delegate using constructor with args. It's not necessarily lucene codec implementation. Currently if user uses this constructor and passes their own codec we will take doc value and compound formats from that delegate.

Signed-off-by: Naveen Tatikonda <[email protected]>

martin-gaievski

let's address concerns raised here in next PR

opensearch-trigger-bot · 2024-01-11T23:27:13Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-1383-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 45e9e542aef60ef7073ee726e6ac14dec27bfa04
# Push it to GitHub
git push --set-upstream origin backport/backport-1383-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-1383-to-2.x.

* Add Lucene Codec 9.9 Signed-off-by: Naveen Tatikonda <[email protected]> * Fix import statements for Lucene95 Codec Signed-off-by: Naveen Tatikonda <[email protected]> * Fix SegmentInfo Constructor in Test Signed-off-by: Naveen Tatikonda <[email protected]> * Temporarily Ignore Old Codec Tests Signed-off-by: Naveen Tatikonda <[email protected]> * Add CHANGELOG Signed-off-by: Naveen Tatikonda <[email protected]> * Delete Old Codec Tests Signed-off-by: Naveen Tatikonda <[email protected]> --------- Signed-off-by: Naveen Tatikonda <[email protected]> (cherry picked from commit 45e9e54)

naveentatikonda added 4 commits January 10, 2024 12:02

Add Lucene Codec 9.9

32c42a0

Signed-off-by: Naveen Tatikonda <[email protected]>

Fix import statements for Lucene95 Codec

5316bcc

Signed-off-by: Naveen Tatikonda <[email protected]>

Fix SegmentInfo Constructor in Test

57cfc91

Signed-off-by: Naveen Tatikonda <[email protected]>

Temporarily Ignore Old Codec Tests

20af1b5

Signed-off-by: Naveen Tatikonda <[email protected]>

naveentatikonda added the Maintenance Add support for new versions of OpenSearch/Dashboards from upstream label Jan 10, 2024

naveentatikonda requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, junqiu-lei, martin-gaievski and ryanbogan as code owners January 10, 2024 22:58

Add CHANGELOG

9d5b7e4

Signed-off-by: Naveen Tatikonda <[email protected]>

martin-gaievski reviewed Jan 11, 2024

View reviewed changes

src/main/java/org/opensearch/knn/index/codec/KNN990Codec/KNN990Codec.java Show resolved Hide resolved

Hailong-am mentioned this pull request Jan 11, 2024

feature: tune description on monitor and anomaly detection opensearch-project/skills#120

Merged

5 tasks

vibrantvarun mentioned this pull request Jan 11, 2024

BWC tests for Multimodal Search, Hybrid Search and Neural Sparse Search opensearch-project/neural-search#533

Merged

5 tasks

ruanyl mentioned this pull request Jan 11, 2024

[BUG] K-NN build failed on main branch with error: cannot find symbol import org.apache.lucene.codecs.lucene95.Lucene95HnswVectorsFormat; #1382

Closed

heemin32 reviewed Jan 11, 2024

View reviewed changes

src/test/java/org/opensearch/knn/index/codec/KNN910Codec/KNN910CodecTests.java Outdated Show resolved Hide resolved

naveentatikonda force-pushed the bump_lucene_codec_to_99 branch from c68c012 to b42a537 Compare January 11, 2024 21:24

naveentatikonda added the backport 2.x label Jan 11, 2024

navneet1v approved these changes Jan 11, 2024

View reviewed changes

Delete Old Codec Tests

accf812

Signed-off-by: Naveen Tatikonda <[email protected]>

naveentatikonda force-pushed the bump_lucene_codec_to_99 branch from b42a537 to accf812 Compare January 11, 2024 23:08

navneet1v mentioned this pull request Jan 11, 2024

Adding support for generic re-ranker interface and opensearch ml re-ranker for improving search relavancy. opensearch-project/neural-search#494

Merged

5 tasks

martin-gaievski approved these changes Jan 11, 2024

View reviewed changes

naveentatikonda merged commit 45e9e54 into opensearch-project:main Jan 11, 2024
21 of 49 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump lucene codec to 99 #1383

Bump lucene codec to 99 #1383

naveentatikonda commented Jan 10, 2024 •

edited

Loading

navneet1v commented Jan 10, 2024

codecov bot commented Jan 10, 2024 •

edited

Loading

naveentatikonda commented Jan 10, 2024

navneet1v commented Jan 10, 2024

martin-gaievski left a comment

navneet1v commented Jan 11, 2024 •

edited

Loading

martin-gaievski commented Jan 11, 2024

martin-gaievski commented Jan 11, 2024

vibrantvarun commented Jan 11, 2024

navneet1v commented Jan 11, 2024

heemin32 commented Jan 11, 2024 •

edited

Loading

navneet1v commented Jan 11, 2024

martin-gaievski commented Jan 11, 2024

martin-gaievski commented Jan 11, 2024

martin-gaievski left a comment

opensearch-trigger-bot bot commented Jan 11, 2024

Bump lucene codec to 99 #1383

Bump lucene codec to 99 #1383

Conversation

naveentatikonda commented Jan 10, 2024 • edited Loading

Description

Check List

navneet1v commented Jan 10, 2024

codecov bot commented Jan 10, 2024 • edited Loading

Codecov Report

naveentatikonda commented Jan 10, 2024

navneet1v commented Jan 10, 2024

martin-gaievski left a comment

Choose a reason for hiding this comment

navneet1v commented Jan 11, 2024 • edited Loading

martin-gaievski commented Jan 11, 2024

martin-gaievski commented Jan 11, 2024

vibrantvarun commented Jan 11, 2024

navneet1v commented Jan 11, 2024

heemin32 commented Jan 11, 2024 • edited Loading

navneet1v commented Jan 11, 2024

martin-gaievski commented Jan 11, 2024

martin-gaievski commented Jan 11, 2024

martin-gaievski left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Jan 11, 2024

naveentatikonda commented Jan 10, 2024 •

edited

Loading

codecov bot commented Jan 10, 2024 •

edited

Loading

navneet1v commented Jan 11, 2024 •

edited

Loading

heemin32 commented Jan 11, 2024 •

edited

Loading