[GitHub] [lucene-solr] dsmiley commented on a change in pull request #2365: LUCENE-9762: DoubleValuesSource.fromQuery bug

[GitBox <gi...@apache.org>]

dsmiley commented on a change in pull request #2365:
URL: https://github.com/apache/lucene-solr/pull/2365#discussion_r576548809



##########
File path: lucene/core/src/java/org/apache/lucene/search/DoubleValuesSource.java
##########
@@ -617,8 +618,17 @@ public double doubleValue() throws IOException {
         public boolean advanceExact(int doc) throws IOException {
           if (disi.docID() < doc) {
             disi.advance(doc);
+            tpiMatch = null;
           }
-          return disi.docID() == doc && (tpi == null || tpi.matches());
+          if (disi.docID() == doc) {

Review comment:
       At first glance, this looked simpler and correct, but on further reflection, I think this contains a bug.  Lets say advanceExact doc==1 and we call disi.advance(1) and it arrives at 2 because the disi doesn't match 1.  We'd then set matched=false and return (fine; it doesn't match doc==1).  Then say the caller calls advanceExact doc==2.  We'd quick return "matched" which is false.  But that's a mistake because the disi in fact matches doc==2 (ignoring tpi; say it's null).
   
   I like the Boolean tpiMatch because it caches the very thing that needs to be cached, and it holds null to say we don't know or it doesn't matter.




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

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