LUCENE-10183: KnnVectorsWriter#writeField to take KnnVectorsReader instead of VectorValues #534

zacharymorn · 2021-12-11T04:24:25Z

Description

KnnVectorsWriter#writeField to take KnnVectorsReader instead of VectorValues

Tests

Passed existing tests via ./gradlew clean; ./gradlew check

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Lucene maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.

…stead of VectorValues

…e-KnnVectorsReader

zacharymorn · 2021-12-11T04:25:29Z

lucene/core/src/java/org/apache/lucene/index/EmptyKnnVectorsReader.java

+import org.apache.lucene.util.Bits;
+
+/** Abstract base class implementing a {@link KnnVectorsReader} that has no vector values. */
+public abstract class EmptyKnnVectorsReader extends KnnVectorsReader {


This class mirrors https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/index/EmptyDocValuesProducer.java for doc values.

I think it's kind of funny that the only way we use it here, it's not actually empty, due to overriding, but it's OK, naming has already been established; let's be consistent.

We do it for doc values because doc values support 5 different types (numeric, sorted numeric, sorted, sorted set, binary) and the empty producer helps only implement the doc values type that we care about. Since there is a single type of vectors, I don't think we need this empty producer, let's remove it and extend KnnVectorsReader directly?

Ah makes sense. Thanks for the context! I've removed it and extended directly.

msokolov · 2021-12-12T21:47:10Z

Maybe also and a CHANGELOG entry? I guess it's a somewhat internal API, being in codecs, but it's not marked @experimental, so if someone had implemented their own KnnVectorsWriter codec, they'd need to change it ...

msokolov · 2021-12-12T21:39:05Z

lucene/core/src/java/org/apache/lucene/index/EmptyKnnVectorsReader.java

+import org.apache.lucene.util.Bits;
+
+/** Abstract base class implementing a {@link KnnVectorsReader} that has no vector values. */
+public abstract class EmptyKnnVectorsReader extends KnnVectorsReader {


I think it's kind of funny that the only way we use it here, it's not actually empty, due to overriding, but it's OK, naming has already been established; let's be consistent.

jtibshirani

This looks good to me, with a small comment around naming.

jtibshirani · 2021-12-13T20:33:17Z

lucene/core/src/java/org/apache/lucene/codecs/KnnVectorsWriter.java

@@ -40,7 +41,8 @@
  protected KnnVectorsWriter() {}

  /** Write all values contained in the provided reader */
-  public abstract void writeField(FieldInfo fieldInfo, VectorValues values) throws IOException;
+  public abstract void writeField(FieldInfo fieldInfo, KnnVectorsReader knnVectorReader)


Small comment, we use both vectorsReader and knnVectorReader for this variable. It'd be nice to use one name consistently (with plural "vectors").

Good suggestion! I've updated them to use knnVectorsReader.

jpountz

Thanks @zacharymorn for cleaning this up, I left a few comments.

jpountz · 2021-12-14T08:11:14Z

lucene/core/src/java/org/apache/lucene/index/EmptyKnnVectorsReader.java

+import org.apache.lucene.util.Bits;
+
+/** Abstract base class implementing a {@link KnnVectorsReader} that has no vector values. */
+public abstract class EmptyKnnVectorsReader extends KnnVectorsReader {


We do it for doc values because doc values support 5 different types (numeric, sorted numeric, sorted, sorted set, binary) and the empty producer helps only implement the doc values type that we care about. Since there is a single type of vectors, I don't think we need this empty producer, let's remove it and extend KnnVectorsReader directly?

jpountz · 2021-12-14T08:15:58Z

lucene/core/src/java/org/apache/lucene/index/VectorValuesWriter.java

+        new EmptyKnnVectorsReader() {
+          @Override
+          public VectorValues getVectorValues(String field) throws IOException {
+            return sortMap != null ? new SortingVectorValues(vectorValues, sortMap) : vectorValues;


This is incorrect as it would return the same instance every time it is called. We should instantiate a new BufferedVectorValues instance every time this method is called.

Oh, good catch! I think with the current usage it probably never matters since we only call this once and dispose of the reader, but it would be a trap waiting for some future usage.

Ops sorry good catch! I've fixed it.

…consistent, and fix vector values reuse bug

zacharymorn · 2021-12-15T05:03:51Z

Maybe also and a CHANGELOG entry? I guess it's a somewhat internal API, being in codecs, but it's not marked @experimental, so if someone had implemented their own KnnVectorsWriter codec, they'd need to change it ...

Just added an entry into 9.1.0. Shall I also add @lucene.experimental for it as well in case it needs to change again? But I don't know about this API enough to foresee that need...

zacharymorn · 2021-12-15T05:04:35Z

Thanks @msokolov @jtibshirani @jpountz for the review and suggestions! I've pushed an update to address the feedback.

zacharymorn · 2021-12-19T20:45:08Z

Hi @jpountz @msokolov @jtibshirani , just want to check back on this and see if the changes look good?

msokolov

I looked it over; looks ok to me. Personally I don't think it's necessary to acknowledge casual reviewers in the CHANGES, but thank you!

zacharymorn · 2022-01-03T06:42:27Z

I looked it over; looks ok to me. Personally I don't think it's necessary to acknowledge casual reviewers in the CHANGES, but thank you!

Happy new year! Thanks @msokolov for the approval! I'll merge the PR in a few days if there's no further feedback from others.

…e-KnnVectorsReader

jpountz · 2022-01-03T13:39:37Z

lucene/core/src/java/org/apache/lucene/codecs/KnnVectorsWriter.java

+
+          @Override
+          public VectorValues getVectorValues(String field) throws IOException {
+            return new VectorValuesMerger(subs, mergeState);


This way, the VectorValues in subs would be shared across all callers of getVectorValues. I think we need to re-create subs every time getVectorValues is called.

Ops sorry, I should have realized this when fixing for VectorValuesWriter. I've fixed this one as well and asserted for new VectorValues to be created upon calls in a test.

…e-KnnVectorsReader

jpountz · 2022-01-05T14:51:42Z

lucene/core/src/test/org/apache/lucene/codecs/perfield/TestPerFieldKnnVectorsFormat.java

          fieldsWritten.add(fieldInfo.name);
-          writer.writeField(fieldInfo, values);
+          // assert that knnVectorsReader#getVectorValues returns different instances upon repeated
+          // calls


This is the sort of things that we usually check via AssertingKnnVectorsReader.

Ah right. I've moved it to AssertingKnnVectorsReader.

…e-KnnVectorsReader

zacharymorn · 2022-01-07T06:14:09Z

Thanks again for the review and approval @jpountz @msokolov @jtibshirani !

…stead of VectorValues (#534)

jtibshirani · 2022-01-18T22:10:58Z

@zacharymorn as a heads up, I just backported this to branch_9x (which I think was your original intention since it's listed under 9.1 in the changelog).

zacharymorn · 2022-01-31T07:16:01Z

@zacharymorn as a heads up, I just backported this to branch_9x (which I think was your original intention since it's listed under 9.1 in the changelog).

Sorry I have been away for a bit. Yes that's my intention, thanks @jtibshirani for the backporting!

zacharymorn added 2 commits December 10, 2021 20:10

LUCENE-10183: KnnVectorsWriter#writeField to take KnnVectorsReader in…

066717a

…stead of VectorValues

Merge branch 'main' into LUCENE-10183-KnnVectorsWriter-WriteField-Tak…

bee502d

…e-KnnVectorsReader

zacharymorn requested review from jpountz, msokolov, mocobeta and jtibshirani December 11, 2021 04:24

zacharymorn commented Dec 11, 2021

View reviewed changes

msokolov approved these changes Dec 12, 2021

View reviewed changes

jtibshirani approved these changes Dec 13, 2021

View reviewed changes

jpountz reviewed Dec 14, 2021

View reviewed changes

Address feedback - remove EmptyKnnVectorsReader, make variable names …

b2d2d3d

…consistent, and fix vector values reuse bug

zacharymorn requested review from jpountz, jtibshirani and msokolov December 15, 2021 05:04

msokolov approved these changes Dec 21, 2021

View reviewed changes

Merge branch 'main' into LUCENE-10183-KnnVectorsWriter-WriteField-Tak…

d741a53

…e-KnnVectorsReader

jpountz requested changes Jan 3, 2022

View reviewed changes

zacharymorn added 2 commits January 3, 2022 16:33

Address feedback - recreate subs in KnnVectorsWriter#mergeVectors

9308441

Merge branch 'main' into LUCENE-10183-KnnVectorsWriter-WriteField-Tak…

c9c2b0e

…e-KnnVectorsReader

zacharymorn requested review from jpountz and msokolov January 4, 2022 04:26

jpountz approved these changes Jan 5, 2022

View reviewed changes

zacharymorn added 2 commits January 5, 2022 20:39

Move assertion to AssertingKnnVectorsFormat

f957ea1

Merge branch 'main' into LUCENE-10183-KnnVectorsWriter-WriteField-Tak…

2706633

…e-KnnVectorsReader

zacharymorn requested a review from jpountz January 6, 2022 04:57

jpountz approved these changes Jan 6, 2022

View reviewed changes

zacharymorn merged commit d0ad9f5 into apache:main Jan 7, 2022

jtibshirani pushed a commit that referenced this pull request Jan 18, 2022

LUCENE-10183: KnnVectorsWriter#writeField to take KnnVectorsReader in…

af3a0bc

…stead of VectorValues (#534)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LUCENE-10183: KnnVectorsWriter#writeField to take KnnVectorsReader instead of VectorValues #534

LUCENE-10183: KnnVectorsWriter#writeField to take KnnVectorsReader instead of VectorValues #534

zacharymorn commented Dec 11, 2021

zacharymorn Dec 11, 2021

msokolov Dec 12, 2021

jpountz Dec 14, 2021

zacharymorn Dec 15, 2021

msokolov commented Dec 12, 2021

msokolov Dec 12, 2021

jtibshirani left a comment

jtibshirani Dec 13, 2021

zacharymorn Dec 15, 2021

jpountz left a comment

jpountz Dec 14, 2021

jpountz Dec 14, 2021

msokolov Dec 14, 2021

zacharymorn Dec 15, 2021

zacharymorn commented Dec 15, 2021

zacharymorn commented Dec 15, 2021

zacharymorn commented Dec 19, 2021

msokolov left a comment

zacharymorn commented Jan 3, 2022

jpountz Jan 3, 2022

zacharymorn Jan 4, 2022

jpountz Jan 5, 2022

zacharymorn Jan 6, 2022

zacharymorn commented Jan 7, 2022

jtibshirani commented Jan 18, 2022

zacharymorn commented Jan 31, 2022

LUCENE-10183: KnnVectorsWriter#writeField to take KnnVectorsReader instead of VectorValues #534

LUCENE-10183: KnnVectorsWriter#writeField to take KnnVectorsReader instead of VectorValues #534

Conversation

zacharymorn commented Dec 11, 2021

Description

Tests

Checklist

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

msokolov commented Dec 12, 2021

Choose a reason for hiding this comment

jtibshirani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpountz left a 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

zacharymorn commented Dec 15, 2021

zacharymorn commented Dec 15, 2021

zacharymorn commented Dec 19, 2021

msokolov left a comment

Choose a reason for hiding this comment

zacharymorn commented Jan 3, 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

zacharymorn commented Jan 7, 2022

jtibshirani commented Jan 18, 2022

zacharymorn commented Jan 31, 2022