You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "benwtrent (via GitHub)" <gi...@apache.org> on 2024/01/22 14:45:01 UTC

[PR] Fix NPE when sampling for quantization in Lucene99HnswScalarQuantizedVectorsFormat [lucene]

benwtrent opened a new pull request, #13027:
URL: https://github.com/apache/lucene/pull/13027

   When merging `Lucene99HnswScalarQuantizedVectorsFormat` a NPE is possible when deleted documents are present.
   
   `ScalarQuantizer#fromVectors` doesn't take deleted documents into account. This means using `FloatVectorValues#size` may actually be larger than the actual size of live documents. Consequently, when iterating for sampling iteration too far is possible and an NPE will be thrown.


-- 
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 NPE when sampling for quantization in Lucene99HnswScalarQuantizedVectorsFormat [lucene]

Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty commented on code in PR #13027:
URL: https://github.com/apache/lucene/pull/13027#discussion_r1463016033


##########
lucene/core/src/java/org/apache/lucene/util/ScalarQuantizer.java:
##########
@@ -201,13 +233,26 @@ public String toString() {
    *
    * @param floatVectorValues the float vector values from which to calculate the quantiles
    * @param confidenceInterval the confidence interval used to calculate the quantiles
+   * @param totalVectorCount the total number of live float vectors in the index. This is vital for
+   *     accounting for deleted documents when calculating the quantiles.
    * @return A new {@link ScalarQuantizer} instance
    * @throws IOException if there is an error reading the float vector values
    */
   public static ScalarQuantizer fromVectors(
-      FloatVectorValues floatVectorValues, float confidenceInterval) throws IOException {
+      FloatVectorValues floatVectorValues, float confidenceInterval, int totalVectorCount)

Review Comment:
   Alternatively, and if we're concerned about binary compatibility with 9.9.0, then this could be structured as a new additional override accepting the `totalVectorCount`, while retaining the the original signature simply calls the new overloaded method passing the `floatVectorValues.size()` and asserting that there are no deleted docs or something. This could be a bit "trappy", but no unreasonable if we think that we need to maintain binary compatibility.



-- 
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 NPE when sampling for quantization in Lucene99HnswScalarQuantizedVectorsFormat [lucene]

Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on PR #13027:
URL: https://github.com/apache/lucene/pull/13027#issuecomment-1904174622

   Good catch. I wonder what is the best place to compute size() correctly. I see you fixed the merge instances, but this is not how it's done elsewhere, see e.g. `FieldsConsumer#write`, which clearly states that index statistics must not be pulled from the merged `Terms` instance but recomputed. Should we follow a similar approach and re-compute the size in `ScalarQuantizer#fromVectors`? I see that it needs to linearly scan all vectors anyway, so this shouldn't come at a performance penalty?


-- 
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 NPE when sampling for quantization in Lucene99HnswScalarQuantizedVectorsFormat [lucene]

Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty commented on code in PR #13027:
URL: https://github.com/apache/lucene/pull/13027#discussion_r1463192412


##########
lucene/CHANGES.txt:
##########
@@ -243,6 +243,13 @@ Other
 
 * GITHUB#12934: Cleaning up old references to Lucene/Solr. (Jakub Slowinski)
 
+======================== Lucene 9.9.2 =======================
+
+Bug Fixes
+---------------------
+
+* GITHUB#13027: Fix NPE when sampling for quantization in Lucene99HnswScalarQuantizedVectorsFormat (Ben Trent)

Review Comment:
   After given it some thought, I agree with targeting this for 9.9.2. This kinda bug is exactly why we have patch releases.



-- 
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 NPE when sampling for quantization in Lucene99HnswScalarQuantizedVectorsFormat [lucene]

Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent merged PR #13027:
URL: https://github.com/apache/lucene/pull/13027


-- 
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 NPE when sampling for quantization in Lucene99HnswScalarQuantizedVectorsFormat [lucene]

Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #13027:
URL: https://github.com/apache/lucene/pull/13027#issuecomment-1904205095

   > Adding an @Override to MergedVectorValues that asserts size() is never called is a good protection idea.
   
   Well, that would be overkill actually, many things use `size()` and use it sanely. Just not me apparently.


-- 
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 NPE when sampling for quantization in Lucene99HnswScalarQuantizedVectorsFormat [lucene]

Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13027:
URL: https://github.com/apache/lucene/pull/13027#discussion_r1463174260


##########
lucene/core/src/java/org/apache/lucene/util/ScalarQuantizer.java:
##########
@@ -201,13 +233,26 @@ public String toString() {
    *
    * @param floatVectorValues the float vector values from which to calculate the quantiles
    * @param confidenceInterval the confidence interval used to calculate the quantiles
+   * @param totalVectorCount the total number of live float vectors in the index. This is vital for
+   *     accounting for deleted documents when calculating the quantiles.
    * @return A new {@link ScalarQuantizer} instance
    * @throws IOException if there is an error reading the float vector values
    */
   public static ScalarQuantizer fromVectors(
-      FloatVectorValues floatVectorValues, float confidenceInterval) throws IOException {
+      FloatVectorValues floatVectorValues, float confidenceInterval, int totalVectorCount)

Review Comment:
   Yeah, changing a public definition in a bug fix stinks. I can overload it for the bug fix and remove the overload in a minor release. These are experimental, so we should be able to change them in minors.



-- 
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 NPE when sampling for quantization in Lucene99HnswScalarQuantizedVectorsFormat [lucene]

Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #13027:
URL: https://github.com/apache/lucene/pull/13027#issuecomment-1904192755

   @jpountz 
   
   > I see that it needs to linearly scan all vectors anyway, so this shouldn't come at a performance penalty?
   
   We would then need to linearly scan all the vectors twice. Because we do not have random access, we can only sample while iterating directly and we don't want to read&copy in vectors that ultimately don't get sample anyways (this would happen if we randomly sampled while iterating).
   
   So, we would need to iterate them twice:
   
    - Once to get the size
    - Then again to sample the vectors
   
   Instead of passing in two iterators, passing in a live vector count seemed more sensible as this is already provided.
   
   
   
   Adding an `@Override` to `MergedVectorValues` that asserts `size()` is never called is a good protection idea.


-- 
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 NPE when sampling for quantization in Lucene99HnswScalarQuantizedVectorsFormat [lucene]

Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #13027:
URL: https://github.com/apache/lucene/pull/13027#issuecomment-1904325653

   @jpountz, I gotcha, pushed a change
   


-- 
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 NPE when sampling for quantization in Lucene99HnswScalarQuantizedVectorsFormat [lucene]

Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty commented on code in PR #13027:
URL: https://github.com/apache/lucene/pull/13027#discussion_r1463026638


##########
lucene/CHANGES.txt:
##########
@@ -243,6 +243,13 @@ Other
 
 * GITHUB#12934: Cleaning up old references to Lucene/Solr. (Jakub Slowinski)
 
+======================== Lucene 9.9.2 =======================
+
+Bug Fixes
+---------------------
+
+* GITHUB#13027: Fix NPE when sampling for quantization in Lucene99HnswScalarQuantizedVectorsFormat (Ben Trent)

Review Comment:
   This is a significant issue and needs addressing with some urgency. However, we need to decide exactly where this should land.



-- 
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 NPE when sampling for quantization in Lucene99HnswScalarQuantizedVectorsFormat [lucene]

Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty commented on code in PR #13027:
URL: https://github.com/apache/lucene/pull/13027#discussion_r1463016033


##########
lucene/core/src/java/org/apache/lucene/util/ScalarQuantizer.java:
##########
@@ -201,13 +233,26 @@ public String toString() {
    *
    * @param floatVectorValues the float vector values from which to calculate the quantiles
    * @param confidenceInterval the confidence interval used to calculate the quantiles
+   * @param totalVectorCount the total number of live float vectors in the index. This is vital for
+   *     accounting for deleted documents when calculating the quantiles.
    * @return A new {@link ScalarQuantizer} instance
    * @throws IOException if there is an error reading the float vector values
    */
   public static ScalarQuantizer fromVectors(
-      FloatVectorValues floatVectorValues, float confidenceInterval) throws IOException {
+      FloatVectorValues floatVectorValues, float confidenceInterval, int totalVectorCount)

Review Comment:
   Alternatively, and if we're concerned about binary compatibility with 9.9.0, then this could be structured as a new additional override accepting the `totalVectorCount`, while retaining the the original signature that passes the `floatVectorValues.size()` and asserting that there are no deleted docs or something. This could be a bit "trappy", but no unreasonable if we think that we need to maintain binary compatibility.



-- 
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 NPE when sampling for quantization in Lucene99HnswScalarQuantizedVectorsFormat [lucene]

Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on PR #13027:
URL: https://github.com/apache/lucene/pull/13027#issuecomment-1904218681

   OK, taking an int in that method sounds fine to me. Can we still preserve the signature of `mergeOneField` to return `void` and re-compute the doc count before passing the doc count to `ScalarQuantizer#fromVectors`? Returning the doc count feels a bit arbitrary.


-- 
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