[GitHub] [lucene] msokolov commented on a diff in pull request #12560: Defer #advanceExact on expression dependencies until their values are needed

["msokolov (via GitHub)" <gi...@apache.org>]

msokolov commented on code in PR #12560:
URL: https://github.com/apache/lucene/pull/12560#discussion_r1327797753


##########
lucene/expressions/src/java/org/apache/lucene/expressions/ExpressionFunctionValues.java:
##########
@@ -39,21 +39,21 @@ class ExpressionFunctionValues extends DoubleValues {
   }
 
   @Override
-  public boolean advanceExact(int doc) {
+  public boolean advanceExact(int doc) throws IOException {
     if (currentDoc == doc) {
       return true;
     }
+    for (DoubleValues v : functionValues) {
+      v.advanceExact(doc);

Review Comment:
   I'm still a bit confused - it seems like we're pushing things around, but I know you have some compelling-looking results so it must be OK? But I think it would be great if you could distill whatever knowledge you've garnered into a comment here. The missing link for me is -- what is ExpressionFunctionValues representing? Is that used for functions only (like `max()`) or generally for any expression? I guess if it's only for functions then it makes sense that we are now getting lazy advance for other expressions that might not need all their subs.  But then have we lost some laziness for functions? Does this work for short-circuited booleans to? Like `(1 == 1) || expensive()`?



##########
lucene/expressions/src/java/org/apache/lucene/expressions/ExpressionValueSource.java:
##########
@@ -90,16 +90,24 @@ public DoubleValues getValues(LeafReaderContext readerContext, DoubleValues scor
   static DoubleValues zeroWhenUnpositioned(DoubleValues in) {
     return new DoubleValues() {
 
-      boolean positioned = false;
+      int currentDoc = -1;
+      double value;
+      boolean computed = false;
 
       @Override
       public double doubleValue() throws IOException {
-        return positioned ? in.doubleValue() : 0;
+        if (computed == false) {
+          value = in.advanceExact(currentDoc) ? in.doubleValue() : 0;
+          computed = true;
+        }
+        return value;
       }
 
       @Override
-      public boolean advanceExact(int doc) throws IOException {
-        return positioned = in.advanceExact(doc);
+      public boolean advanceExact(int doc) {
+        currentDoc = doc;

Review Comment:
   is there any sense in checking if we are advancing to the same doc?



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