[GitHub] [lucene] jpountz commented on a diff in pull request #780: LUCENE-10496: avoid unnecessary attempts to evaluate skipping doc if index sort and search sort are in opposite direction

[GitBox]

jpountz commented on code in PR #780:
URL: https://github.com/apache/lucene/pull/780#discussion_r869932548


##########
lucene/core/src/java/org/apache/lucene/search/comparators/NumericComparator.java:
##########
@@ -269,11 +276,23 @@ public PointValues.Relation compare(byte[] 
minPackedValue, byte[] maxPackedValue
       if (estimatedNumberOfMatches >= threshold) {
         // the new range is not selective enough to be worth materializing, it 
doesn't reduce number
         // of docs at least 8x
+        if (updateCounter > 256) {
+          if (tryUpdateFailCount >= 3) {
+            currentSkipInterval = Math.min(currentSkipInterval * 2, 
MAX_SKIP_INTERVAL);
+            tryUpdateFailCount = 0;
+          } else {
+            tryUpdateFailCount++;
+          }
+        }

Review Comment:
   this logic is not entirely trivial, so inlining it here makes the code a bit 
hard to read, can you maybe extract it into an `updateSkipInterval(boolean 
success)` helper method that would encapsulate it, plus the other code addition 
you made below, or something along these lines?



##########
lucene/core/src/java/org/apache/lucene/search/comparators/NumericComparator.java:
##########
@@ -269,11 +276,23 @@ public PointValues.Relation compare(byte[] 
minPackedValue, byte[] maxPackedValue
       if (estimatedNumberOfMatches >= threshold) {
         // the new range is not selective enough to be worth materializing, it 
doesn't reduce number
         // of docs at least 8x
+        if (updateCounter > 256) {
+          if (tryUpdateFailCount >= 3) {
+            currentSkipInterval = Math.min(currentSkipInterval * 2, 
MAX_SKIP_INTERVAL);
+            tryUpdateFailCount = 0;
+          } else {
+            tryUpdateFailCount++;
+          }
+        }

Review Comment:
   Maybe also add a comment about `tryUpdateFailCount` that explains that it 
helps be conservative about increasing the sampling interval?



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