kaivalnp commented on code in PR #14932:
URL: https://github.com/apache/lucene/pull/14932#discussion_r2248977825
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsWriter.java:
##########
@@ -342,8 +356,12 @@ private void reconstructAndWriteNeighbours(
}
// Write the size after duplicates are removed
vectorIndex.writeVInt(actualSize);
- for (int i = 0; i < actualSize; i++) {
- vectorIndex.writeVInt(scratch[i]);
+ if (version >= Lucene99HnswVectorsFormat.VERSION_GROUPVARINT) {
Review Comment:
nit: import as static constant for parity?
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsFormat.java:
##########
@@ -137,11 +138,22 @@ public final class Lucene99HnswVectorsFormat extends
KnnVectorsFormat {
private final int numMergeWorkers;
private final TaskExecutor mergeExec;
+ private final int writeVersion;
+
/** Constructs a format using default graph construction parameters */
public Lucene99HnswVectorsFormat() {
this(DEFAULT_MAX_CONN, DEFAULT_BEAM_WIDTH, DEFAULT_NUM_MERGE_WORKER, null);
}
+ /**
+ * Constructs a format using the default parameters and the specific writer
version.
+ *
+ * @param writeVersion the version used for the writer to encode docID's
(VarInt=0, GroupVarInt=1)
+ */
+ Lucene99HnswVectorsFormat(int writeVersion) {
+ this(DEFAULT_MAX_CONN, DEFAULT_BEAM_WIDTH, DEFAULT_NUM_MERGE_WORKER, null,
writeVersion);
+ }
+
Review Comment:
I think we can avoid this constructor and use the larger one below from
tests? (all defaults are `public`)
If you do so, we should add a "test-only" comment to that constructor.
If you still want to keep this, we should add the "test-only" comment here
and make that `private`? (since it isn't used outside this class).
##########
lucene/core/src/test/org/apache/lucene/codecs/lucene99/TestLucene99HnswVectorsFormatV2.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.codecs.lucene99;
+
+import org.apache.lucene.codecs.Codec;
+import org.apache.lucene.tests.index.BaseKnnVectorsFormatTestCase;
+import org.apache.lucene.tests.util.TestUtil;
+
+public class TestLucene99HnswVectorsFormatV2 extends
BaseKnnVectorsFormatTestCase {
Review Comment:
nit: Rename as `TestLucene99HnswVectorsFormatV0` since we're checking minor
version `0` here?
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsFormat.java:
##########
@@ -165,6 +177,27 @@ public Lucene99HnswVectorsFormat(int maxConn, int
beamWidth) {
*/
public Lucene99HnswVectorsFormat(
int maxConn, int beamWidth, int numMergeWorkers, ExecutorService
mergeExec) {
+ this(maxConn, beamWidth, numMergeWorkers, mergeExec, 1);
Review Comment:
We should use `VERSION_CURRENT` instead of `1` here
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsFormat.java:
##########
@@ -165,6 +177,27 @@ public Lucene99HnswVectorsFormat(int maxConn, int
beamWidth) {
*/
public Lucene99HnswVectorsFormat(
int maxConn, int beamWidth, int numMergeWorkers, ExecutorService
mergeExec) {
+ this(maxConn, beamWidth, numMergeWorkers, mergeExec, 1);
+ }
+
+ /**
+ * Constructs a format using the given graph construction parameters and
scalar quantization.
Review Comment:
`scalar quantization` seems incorrect here? Although I see it was
pre-existing..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
