You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "Pranshu-S (via GitHub)" <gi...@apache.org> on 2026/10/02 09:14:57 UTC
[PR] Fix Lucene104ScalarQuantizedVectorsWriter to skip deleted docs when recomputing merge centroid [lucene]
Pranshu-S opened a new pull request, #16764:
URL: https://github.com/apache/lucene/pull/16764
### Description
<!--
If this is your first contribution to Lucene, please make sure you have reviewed the contribution guide.
https://github.com/apache/lucene/blob/main/CONTRIBUTING.md
-->
Lucene104ScalarQuantizedVectorsWriter computes a per-field centroid before quantizing full-precision vectors during a merge. The centroid is the mean of the full-precision vectors gathered from the segments being merged, and it is later used as the reference point for scalar quantization of every vector in the merged segment.
When a source segment has deletions, the recalculation path iterated over the segment's FloatVectorValues without consulting `MergeState.liveDocs`, so deleted documents were still summed into the centroid. This skewed the centroid toward deleted data and degraded the quantization of every live vector in the merged segment.
This change threads the per-reader liveDocs bit into accumulateCentroid and skips any doc that is not live, addressing the issue.
Fixes issue - #16762
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix Lucene104ScalarQuantizedVectorsWriter to skip deleted docs when recomputing merge centroid [lucene]
Posted by "kaivalnp (via GitHub)" <gi...@apache.org>.
kaivalnp commented on code in PR #16764:
URL: https://github.com/apache/lucene/pull/16764#discussion_r4186215711
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene104/Lucene104ScalarQuantizedVectorsWriter.java:
##########
@@ -642,7 +643,7 @@ static int calculateCentroid(MergeState mergeState, FieldInfo fieldInfo, float[]
for (int i = 0; i < mergeState.knnVectorsReaders.length; i++) {
KnnVectorsReader knnVectorsReader = mergeState.knnVectorsReaders[i];
if (knnVectorsReader == null) continue;
- count += accumulateCentroid(knnVectorsReader, fieldInfo, centroid);
+ count += accumulateCentroid(knnVectorsReader, fieldInfo, mergeState.liveDocs[i], centroid);
Review Comment:
Can you update the changelog for the second one too? (i.e. "reduced cases of preparing unnecessary query data, by making checks consistent b/w flat and HNSW writers")
Can you also update [this javadoc](https://github.com/apache/lucene/blob/3970208e0d4d5e997a10baac816bd7d1d7693350/lucene/core/src/java/org/apache/lucene/codecs/hnsw/FlatVectorsWriter.java#L82-L84)?
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix Lucene104ScalarQuantizedVectorsWriter to skip deleted docs when recomputing merge centroid [lucene]
Posted by "Pranshu-S (via GitHub)" <gi...@apache.org>.
Pranshu-S commented on PR #16764:
URL: https://github.com/apache/lucene/pull/16764#issuecomment-6060717897
@kaivalnp are we good to go ahead and merge the PR? I believe once merged I would need to raise a backport PR to https://github.com/apache/lucene/tree/branch_10x?
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix Lucene104ScalarQuantizedVectorsWriter to skip deleted docs when recomputing merge centroid [lucene]
Posted by "kaivalnp (via GitHub)" <gi...@apache.org>.
kaivalnp commented on PR #16764:
URL: https://github.com/apache/lucene/pull/16764#issuecomment-6066926770
I took a stab at the backport in #16800
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix Lucene104ScalarQuantizedVectorsWriter to skip deleted docs when recomputing merge centroid [lucene]
Posted by "Pranshu-S (via GitHub)" <gi...@apache.org>.
Pranshu-S commented on code in PR #16764:
URL: https://github.com/apache/lucene/pull/16764#discussion_r4186002608
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene104/Lucene104ScalarQuantizedVectorsWriter.java:
##########
@@ -642,7 +643,7 @@ static int calculateCentroid(MergeState mergeState, FieldInfo fieldInfo, float[]
for (int i = 0; i < mergeState.knnVectorsReaders.length; i++) {
KnnVectorsReader knnVectorsReader = mergeState.knnVectorsReaders[i];
if (knnVectorsReader == null) continue;
- count += accumulateCentroid(knnVectorsReader, fieldInfo, centroid);
+ count += accumulateCentroid(knnVectorsReader, fieldInfo, mergeState.liveDocs[i], centroid);
Review Comment:
Right, looks like this comment needs to be removed now. We fixed two bugs in one go :)
From what I understand, needsMergeScorer is a predicate passed as [Lucene99HnswVectorsWriter::buildsGraph](https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsWriter.java#L711-L713), which checks whether the vector count is high enough to require an HNSW graph. If it is, Lucene prepares [query-side data](https://github.com/apache/lucene/blob/3970208e0d4d5e997a10baac816bd7d1d7693350/lucene/core/src/java/org/apache/lucene/codecs/lucene104/Lucene104ScalarQuantizedVectorsWriter.java#L374-L376) to score vectors during graph construction.
Previously, it could receive a count including deleted vectors, causing unnecessary query-side data preparation even when the live-vector count was below the threshold.
The actual graph-building decision already uses [LiveDocs](https://github.com/apache/lucene/blob/14dacef992690d37d04a7ad816f534835ee1fffe/lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsWriter.java#L518) from the merged segment, which only includes live vectors. With this change, both decisions use the live-vector count, avoiding unnecessary preparation without changing whether a graph is built.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix Lucene104ScalarQuantizedVectorsWriter to skip deleted docs when recomputing merge centroid [lucene]
Posted by "kaivalnp (via GitHub)" <gi...@apache.org>.
kaivalnp commented on PR #16764:
URL: https://github.com/apache/lucene/pull/16764#issuecomment-6066638102
Hi @Pranshu-S, I see merge conflicts while cherry-picking to `branch_10x`, could you open a backport PR?
FWIW the conflicts seem straightforward to resolve, but a PR would be better for visibility.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix Lucene104ScalarQuantizedVectorsWriter to skip deleted docs when recomputing merge centroid [lucene]
Posted by "kaivalnp (via GitHub)" <gi...@apache.org>.
kaivalnp commented on code in PR #16764:
URL: https://github.com/apache/lucene/pull/16764#discussion_r4185761568
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene104/Lucene104ScalarQuantizedVectorsWriter.java:
##########
@@ -642,7 +643,7 @@ static int calculateCentroid(MergeState mergeState, FieldInfo fieldInfo, float[]
for (int i = 0; i < mergeState.knnVectorsReaders.length; i++) {
KnnVectorsReader knnVectorsReader = mergeState.knnVectorsReaders[i];
if (knnVectorsReader == null) continue;
- count += accumulateCentroid(knnVectorsReader, fieldInfo, centroid);
+ count += accumulateCentroid(knnVectorsReader, fieldInfo, mergeState.liveDocs[i], centroid);
Review Comment:
Using the count of live vectors is the correct thing to do [here](https://github.com/apache/lucene/blob/3970208e0d4d5e997a10baac816bd7d1d7693350/lucene/core/src/java/org/apache/lucene/codecs/lucene104/Lucene104ScalarQuantizedVectorsWriter.java#L651), but [this comment](https://github.com/apache/lucene/blob/3970208e0d4d5e997a10baac816bd7d1d7693350/lucene/core/src/java/org/apache/lucene/codecs/lucene104/Lucene104ScalarQuantizedVectorsWriter.java#L363-L365) suggests that `needsMergeScorer` accepts the vector count _before_ deletions?
The [`buildsGraph`](https://github.com/apache/lucene/blob/3970208e0d4d5e997a10baac816bd7d1d7693350/lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsWriter.java#L711-L713) function is used as the `needsMergeScorer` predicate -- in [one place](https://github.com/apache/lucene/blob/3970208e0d4d5e997a10baac816bd7d1d7693350/lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsWriter.java#L435) it accepts the count of vectors before deletions, but in [another place](https://github.com/apache/lucene/blob/3970208e0d4d5e997a10baac816bd7d1d7693350/lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsWriter.java#L451-L452) it accepts count of live vectors written to the new segment?
I'm confused.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix Lucene104ScalarQuantizedVectorsWriter to skip deleted docs when recomputing merge centroid [lucene]
Posted by "kaivalnp (via GitHub)" <gi...@apache.org>.
kaivalnp merged PR #16764:
URL: https://github.com/apache/lucene/pull/16764
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix Lucene104ScalarQuantizedVectorsWriter to skip deleted docs when recomputing merge centroid [lucene]
Posted by "Pulkitg64 (via GitHub)" <gi...@apache.org>.
Pulkitg64 commented on code in PR #16764:
URL: https://github.com/apache/lucene/pull/16764#discussion_r4170862192
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene104/Lucene104ScalarQuantizedVectorsWriter.java:
##########
@@ -642,7 +643,7 @@ static int calculateCentroid(MergeState mergeState, FieldInfo fieldInfo, float[]
for (int i = 0; i < mergeState.knnVectorsReaders.length; i++) {
KnnVectorsReader knnVectorsReader = mergeState.knnVectorsReaders[i];
if (knnVectorsReader == null) continue;
- count += accumulateCentroid(knnVectorsReader, fieldInfo, centroid);
+ count += accumulateCentroid(knnVectorsReader, fieldInfo, mergeState.liveDocs[i], centroid);
Review Comment:
We are changing the behavior here as we are now getting count of live vectors only. Could you please confirm if it doesn't have any consequences?
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix Lucene104ScalarQuantizedVectorsWriter to skip deleted docs when recomputing merge centroid [lucene]
Posted by "Pulkitg64 (via GitHub)" <gi...@apache.org>.
Pulkitg64 commented on code in PR #16764:
URL: https://github.com/apache/lucene/pull/16764#discussion_r4186065114
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene104/Lucene104ScalarQuantizedVectorsWriter.java:
##########
@@ -642,7 +643,7 @@ static int calculateCentroid(MergeState mergeState, FieldInfo fieldInfo, float[]
for (int i = 0; i < mergeState.knnVectorsReaders.length; i++) {
KnnVectorsReader knnVectorsReader = mergeState.knnVectorsReaders[i];
if (knnVectorsReader == null) continue;
- count += accumulateCentroid(knnVectorsReader, fieldInfo, centroid);
+ count += accumulateCentroid(knnVectorsReader, fieldInfo, mergeState.liveDocs[i], centroid);
Review Comment:
> Right, looks like this comment needs to be removed now. We fixed two bugs in one go :)
Wow!! Nicee!!
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org