Refactor KNNCodec to use new extension point #319

jmazanec15 · 2022-03-11T19:04:56Z

Description

This is a fairly large refactor of our current KNNCodec system.

First, it deletes unused code. For instance, we had KNN84Codec and KNN86Codec that we were not using. The reason to keep them had been potential assistance with backwards compatibility. However, given that we do not use them now, I think it makes sense to just delete.

Second, the plugin is refactored to implement getCustomCodecServiceFactory. This is a new extension point in OpenSearch and will allow multiple EnginePlugins to be used on the same index. For instance, the CCR plugin and k-NN. This allows us to delete the EngineFactory code. Along with this, I moved KNNCodecService to the codec folder for consistency.

Third, the KNN87Codec was refactored to implement the FilterCodec. The KNN87Codec uses the delegate pattern to override the DocValuesFormat. The rest of the formats are delegated to a delegate. This is what the FilterCodec is intended to be used for. Along with this, I extended the delegate reach further so that the KNN87Codec could implement the pattern to its fullest potential.

Fourth, I added several unit tests for the individual components of the KNN87Codec as well as a util class. This will allow us to cover to a fuller extent the test coverage around these components. I have not yet added unit test coverage for merging or for the KNNCodecUtil. I will do this in the future.

Note - I expect checkstyle to fail. My plan is to run ./gradlew spotlessApply after I get review so as to not distract reviewers from the substantive changes.

Issues Resolved

#146 #147

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.

Initial commit for plugin to return CodecServiceFactory as opposed to CodecService. This will allow the plugin to make decisions based on Mapper Service. Signed-off-by: John Mazanec <[email protected]>

Refactors the KNN87Codec to implement FilterCodec. This allows the codec to automatically/flexibly delegate operations it does not override to an arbitrary Codec. Additionally cleans up some code around the Codec Signed-off-by: John Mazanec <[email protected]>

Signed-off-by: John Mazanec <[email protected]>

Adds unit tests that map to each codec component. Did not add tests for merging and codec utils. This can be undertaken later. Adds a utils folder for sharing testing functionality between codec tests. Cleans up a few minor details around codec source code. Signed-off-by: John Mazanec <[email protected]>

VijayanB · 2022-03-15T17:13:27Z

src/main/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesConsumer.java


-            if (field.attributes().containsKey(MODEL_ID)) {
+        if (field.attributes().containsKey(MODEL_ID)) {


Do we need null check for attributes before calling contains?

I do not think so. Sole constructor requires non-null: https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/index/FieldInfo.java#L103

martin-gaievski · 2022-03-16T19:35:37Z

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

+     */
+    @Override
+    public Codec codec(String name) {
+        return new KNN87Codec(super.codec(name));


I think we need some sort of factory here that will allow to select exact k-NN Codec implementation. I'm thinking of a use case for 2.0/lucene 9, we'll need to use Lucene90 codec for it, and probably will create new kNN wrapper class, something like KNN90Codec. Although it might be too much for this PR, we can do it in next one.

Im not sure of a case where we will want to build a Codec for an older version. For instance, when we upgrade to Lucene90, I dont think we will want to allow the option to build KNN87Codec.

Ok, I see, so essentially we change it with every next Lucene version but keep older codecs for bwc. Seems that this method (codec(String name)) is the factory method itself. Do we have our abstraction for knn codec apart from lucene abstract Codec class? It may give us more flexibility with testing, although I'm not sure it's that easy to pass our interface to the OpenSearch through plugin related service classes.

Right, so we keep older Codecs so that older indices can be read by old Codecs via SPI.

BWC is also why we need to version our Codecs (KNN87Codec). This is so that older indices can be read with old codecs.

I think this is something that could be investigated more in a future review.

vamshin · 2022-03-16T19:58:46Z

src/main/resources/META-INF/services/org.apache.lucene.codecs.Codec

-org.opensearch.knn.index.codec.KNN80Codec.KNN80Codec
-org.opensearch.knn.index.codec.KNN84Codec.KNN84Codec
-org.opensearch.knn.index.codec.KNN86Codec.KNN86Codec


Are we sure we do not use these codecs in any older opendistro version? If these codecs were used to create segments in some older version then assuming they upgrade to latest OpenSearch version, it might fail as they do not find respective codec for reading.

Good catch. I think this could have problems for SPI.

Let me add back these Codecs back.

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 · 2022-03-17T18:05:54Z

@martin-gaievski @vamshin Failure related to spotless check. Please review again - Ill fix spotless after I get approval

vamshin · 2022-03-17T18:20:30Z

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

+                // If the delegate is an instanceof Lucene87, we can get a field specific doc values format
+                // delegate
+                if (delegate instanceof Lucene87Codec) {


Are we not doing this for every code >= Lucene87? Wondering why this should be only specific to Lucene87

Right, so Codec does not have the method getDocValuesFormatForField. Lucene8XCodec's do, however. So, I felt like it made sense to map this to the LuceneCodec version our KNN codec version maps to.

We can pull this to the separate DocValuesFormat class and inject to knn codec during construction time. We can also make getDocValuesFormatForField part of our codec facade abstraction, this should help to decrease coupling between our and lucene classes. These ideas are more for next refactoring.

@martin-gaievski what would this look like?

I actually was able to remove this code. The reason beingthat the delegate could have the per field functionality and we dont need to do anything to get it.

nice. My initial idea was to create a facade interface for knn codec and then inject it to KNNCodecService using DI similar to spring or guice. I may try to play with it while working on integration with opensearch 2.0/lucene 9.1, we need a codec based on lucene 9.1

Signed-off-by: John Mazanec <[email protected]>

martin-gaievski

lgtm

Signed-off-by: John Mazanec <[email protected]>

vamshin

LGTM! Thanks

Signed-off-by: John Mazanec <[email protected]>

codecov-commenter · 2022-03-17T23:51:02Z

Codecov Report

Merging #319 (47747e4) into main (bb28393) will increase coverage by 0.24%.
The diff coverage is 91.13%.

@@             Coverage Diff              @@
##               main     #319      +/-   ##
============================================
+ Coverage     83.49%   83.74%   +0.24%     
+ Complexity      889      885       -4     
============================================
  Files           127      126       -1     
  Lines          3829     3801      -28     
  Branches        361      360       -1     
============================================
- Hits           3197     3183      -14     
+ Misses          470      457      -13     
+ Partials        162      161       -1

Impacted Files	Coverage Δ
...n/index/codec/KNN80Codec/KNN80BinaryDocValues.java	`100.00% <ø> (+28.57%)`	⬆️
...n/index/codec/KNN80Codec/KNN80DocValuesReader.java	`75.00% <ø> (ø)`
...earch/knn/index/codec/util/BinaryDocValuesSub.java	`71.42% <ø> (ø)`
...index/codec/KNN80Codec/KNN80DocValuesConsumer.java	`88.23% <87.27%> (+6.23%)`	⬆️
...nn/index/codec/KNN80Codec/KNN80CompoundFormat.java	`100.00% <100.00%> (ø)`
...n/index/codec/KNN80Codec/KNN80DocValuesFormat.java	`100.00% <100.00%> (+16.66%)`	⬆️
...nsearch/knn/index/codec/KNN87Codec/KNN87Codec.java	`100.00% <100.00%> (+4.00%)`	⬆️
...rg/opensearch/knn/index/codec/KNNCodecService.java	`100.00% <100.00%> (ø)`
...main/java/org/opensearch/knn/plugin/KNNPlugin.java	`100.00% <100.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb28393...47747e4. Read the comment docs.

vamshin

LGTM! Thanks

Refactor plugin to return CodecServiceFactory as opposed to CodecService. This will allow the plugin to make decisions based on Mapper Service. Refactors the KNN87Codec to implement FilterCodec. This allows the codec to automatically/flexibly delegate operations it does not override to an arbitrary Codec. Additionally cleans up some code around the Codec Adds unit tests that map to each codec component. Did not add tests for merging and codec utils. This can be undertaken later. Adds a utils folder for sharing testing functionality between codec tests. Cleans up a few minor details around codec source code. Signed-off-by: John Mazanec <[email protected]>

jmazanec15 added 4 commits March 9, 2022 10:13

Refactor plugin to return CodecServiceFactory

80e2aa4

Initial commit for plugin to return CodecServiceFactory as opposed to CodecService. This will allow the plugin to make decisions based on Mapper Service. Signed-off-by: John Mazanec <[email protected]>

Move KNNCodecService to codec folder

1cc9f24

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 added Refactoring Improve the design, structure, and implementation while preserving its functionality v2.0.0 labels Mar 11, 2022

jmazanec15 requested a review from a team March 11, 2022 19:04

VijayanB reviewed Mar 15, 2022

View reviewed changes

jmazanec15 requested review from VijayanB and martin-gaievski March 15, 2022 17:42

martin-gaievski reviewed Mar 16, 2022

View reviewed changes

vamshin reviewed Mar 16, 2022

View reviewed changes

jmazanec15 force-pushed the codec-service-refactor branch 3 times, most recently from 11085ec to 8e99dd4 Compare March 17, 2022 16:31

Add back in older codecs for bwc

0845b3d

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 force-pushed the codec-service-refactor branch from 8e99dd4 to 0845b3d Compare March 17, 2022 16:32

jmazanec15 added 2 commits March 17, 2022 09:35

Fix licenses

41ed25f

Signed-off-by: John Mazanec <[email protected]>

Add back no arg ctors

7f5bf23

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 force-pushed the codec-service-refactor branch from df7ae05 to 7f5bf23 Compare March 17, 2022 16:46

Fix compound format ctor

b3db12b

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 requested review from vamshin and martin-gaievski March 17, 2022 18:05

vamshin reviewed Mar 17, 2022

View reviewed changes

jmazanec15 requested a review from vamshin March 17, 2022 19:17

Remove perfield as it is inherited

31f5be9

Signed-off-by: John Mazanec <[email protected]>

martin-gaievski previously approved these changes Mar 17, 2022

View reviewed changes

Switch default to new

e03b574

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 dismissed martin-gaievski’s stale review via e03b574 March 17, 2022 22:42

vamshin previously approved these changes Mar 17, 2022

View reviewed changes

Spotless applied

284eb2a

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 dismissed vamshin’s stale review via 284eb2a March 17, 2022 22:58

Add workaround for spotless diffplug/spotless#834 (comment)

47747e4

Signed-off-by: John Mazanec <[email protected]>

vamshin approved these changes Mar 18, 2022

View reviewed changes

martin-gaievski approved these changes Mar 18, 2022

View reviewed changes

jmazanec15 merged commit d531b3c into opensearch-project:main Mar 18, 2022

jmazanec15 mentioned this pull request Sep 15, 2022

[BUG] "no index mapper found for field" warning appears even if there is no knn-vector mapping #537

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor KNNCodec to use new extension point #319

Refactor KNNCodec to use new extension point #319

jmazanec15 commented Mar 11, 2022

VijayanB Mar 15, 2022

jmazanec15 Mar 15, 2022

martin-gaievski Mar 16, 2022

jmazanec15 Mar 17, 2022

martin-gaievski Mar 17, 2022

jmazanec15 Mar 17, 2022

vamshin Mar 16, 2022

jmazanec15 Mar 17, 2022

jmazanec15 commented Mar 17, 2022

vamshin Mar 17, 2022

jmazanec15 Mar 17, 2022

martin-gaievski Mar 17, 2022

jmazanec15 Mar 17, 2022

jmazanec15 Mar 17, 2022

martin-gaievski Mar 17, 2022

martin-gaievski left a comment

vamshin left a comment

codecov-commenter commented Mar 17, 2022 •

edited

Loading

vamshin left a comment


		if (field.attributes().containsKey(MODEL_ID)) {
		if (field.attributes().containsKey(MODEL_ID)) {

Refactor KNNCodec to use new extension point #319

Refactor KNNCodec to use new extension point #319

Conversation

jmazanec15 commented Mar 11, 2022

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmazanec15 commented Mar 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-gaievski left a comment

Choose a reason for hiding this comment

vamshin left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 17, 2022 • edited Loading

Codecov Report

vamshin left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 17, 2022 •

edited

Loading