Use a more coarse-grained competitive iterator for skipper-based numeric sorts (#15632)

Numeric sorts against a field with DocValuesSkippers enabled currently use
DocValuesRangeIterator to implement competitive iterators. This has a couple
of disadvantages:
- DVRI cannot efficiently implement docIDRunEnd() or intoBitSet(), meaning that
  bulk conjunction filtering may end up falling into slower code paths
- For field value distributions that are essentially random, DVRI falls back to
  doc-by-doc value checking, meaning that no skipping happens at all, but adding
  overhead.

This commit adds a new SkipBlockRangeIterator that only skips whole blocks
where no document will be competitive, avoiding any individual doc-by-doc value
checks. The docIDRunEnd() and intoBitSet() implementations are very fast and
mean that bulk conjunction filtering will be efficient. The overheads as a whole
are very low, so randomly distributed values are much less adversarial, while
queries against indexes where the document order is roughly correlated with the
query sort get significant boosts.

Author: romseygeek

lucene/CHANGES.txt

@@ -336,6 +336,9 @@ Optimizations
 
 * GITHUB#15585: Added shared prefetch counter to maintain count across clones and slices of MemorySegmentIndexInput. (Shubham Sharma)
 
+* GITHUB#15632: Use a coarser-grained competitive iterator with lower construction costs for
+  numeric sorts against fields with DocValuesSkippers. (Alan Woodward)
+
 Bug Fixes
 ---------------------
 * GITHUB#14161: PointInSetQuery's constructor now throws IllegalArgumentException

lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90DocValuesProducer.java

@@ -1895,7 +1895,8 @@ public void advance(int target) throws IOException {
           }
         } else {
           // find next interval
-          assert target > maxDocID[0] : "target must be bigger that current interval";
+          assert target > maxDocID[0]
+              : "target " + target + " must be bigger that current interval " + maxDocID[0];
           while (true) {
             levels = input.readByte();
             assert levels <= SKIP_INDEX_MAX_LEVEL && levels > 0

lucene/core/src/java/org/apache/lucene/search/SkipBlockRangeIterator.java

@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.search;
+
+import java.io.IOException;
+import org.apache.lucene.index.DocValuesSkipper;
+import org.apache.lucene.util.FixedBitSet;
+
+/**
+ * A DocIdSetIterator that returns all documents within DocValuesSkipper blocks that have minimum
+ * and maximum values that fall within a specified range.
+ */
+public class SkipBlockRangeIterator extends AbstractDocIdSetIterator {
+
+  private final DocValuesSkipper skipper;
+  private final long minValue;
+  private final long maxValue;
+
+  /**
+   * Creates a new SkipBlockRangeIterator
+   *
+   * @param skipper the DocValuesSkipper to use to check block bounds
+   * @param minValue only return documents that lie within a block with a maximum value greater than
+   *     this
+   * @param maxValue only return documents that lie within a block with a minimum value less than
+   *     this
+   */
+  public SkipBlockRangeIterator(DocValuesSkipper skipper, long minValue, long maxValue) {
+    this.skipper = skipper;
+    this.minValue = minValue;
+    this.maxValue = maxValue;
+  }
+
+  @Override
+  public int nextDoc() throws IOException {
+    return advance(doc + 1);
+  }
+
+  @Override
+  public int advance(int target) throws IOException {
+    if (target <= skipper.maxDocID(0)) {
+      // within current block
+      if (doc > -1) {
+        // already positioned, so we've checked bounds and know that we're in a matching block
+        return doc = target;
+      }
+    } else {
+      // Advance to target
+      skipper.advance(target);
+    }
+
+    // Find the next matching block (could be the current block)
+    skipper.advance(minValue, maxValue);
+    return doc = Math.max(target, skipper.minDocID(0));
+  }
+
+  @Override
+  public long cost() {
+    return DocIdSetIterator.NO_MORE_DOCS;
+  }
+
+  @Override
+  public int docIDRunEnd() throws IOException {
+    int maxDoc = skipper.maxDocID(0);
+    int nextLevel = 1;
+    while (nextLevel < skipper.numLevels()
+        && skipper.minValue(nextLevel) < maxValue
+        && skipper.maxValue(nextLevel) > minValue) {
+      maxDoc = skipper.maxDocID(nextLevel);
+      nextLevel++;
+    }
+    return maxDoc + 1;
+  }
+
+  @Override
+  public void intoBitSet(int upTo, FixedBitSet bitSet, int offset) throws IOException {
+    while (doc < upTo) {
+      int end = Math.min(upTo, docIDRunEnd());
+      bitSet.set(doc - offset, end - offset);
+      advance(end);
+    }
+  }
+}

lucene/core/src/java/org/apache/lucene/search/comparators/NumericComparator.java

@@ -26,13 +26,12 @@
 import org.apache.lucene.index.NumericDocValues;
 import org.apache.lucene.index.PointValues;
 import org.apache.lucene.search.DocIdSetIterator;
-import org.apache.lucene.search.DocValuesRangeIterator;
 import org.apache.lucene.search.FieldComparator;
 import org.apache.lucene.search.LeafFieldComparator;
 import org.apache.lucene.search.Pruning;
 import org.apache.lucene.search.Scorable;
 import org.apache.lucene.search.Scorer;
-import org.apache.lucene.search.TwoPhaseIterator;
+import org.apache.lucene.search.SkipBlockRangeIterator;
 import org.apache.lucene.util.DocIdSetBuilder;
 import org.apache.lucene.util.IntsRef;
 
@@ -502,27 +501,11 @@ private void updateSkipInterval(boolean success) {
   private class DVSkipperCompetitiveDISIBuilder extends CompetitiveDISIBuilder {
 
     private final DocValuesSkipper skipper;
-    private final TwoPhaseIterator innerTwoPhase;
 
-    DVSkipperCompetitiveDISIBuilder(DocValuesSkipper skipper, NumericLeafComparator leafComparator)
-        throws IOException {
+    DVSkipperCompetitiveDISIBuilder(
+        DocValuesSkipper skipper, NumericLeafComparator leafComparator) {
       super(leafComparator);
       this.skipper = skipper;
-      NumericDocValues docValues =
-          leafComparator.getNumericDocValues(leafComparator.context, field);
-      innerTwoPhase =
-          new TwoPhaseIterator(docValues) {
-            @Override
-            public boolean matches() throws IOException {
-              final long value = docValues.longValue();
-              return value >= minValueAsLong && value <= maxValueAsLong;
-            }
-
-            @Override
-            public float matchCost() {
-              return 2; // 2 comparisons
-            }
-          };
       postInitializeCompetitiveIterator();
     }
 
@@ -553,9 +536,8 @@ void postInitializeCompetitiveIterator() {
 
     @Override
     void doUpdateCompetitiveIterator() {
-      TwoPhaseIterator twoPhaseIterator =
-          new DocValuesRangeIterator(innerTwoPhase, skipper, minValueAsLong, maxValueAsLong, false);
-      competitiveIterator.update(TwoPhaseIterator.asDocIdSetIterator(twoPhaseIterator));
+      competitiveIterator.update(
+          new SkipBlockRangeIterator(skipper, minValueAsLong, maxValueAsLong));
     }
   }
 }

lucene/core/src/test/org/apache/lucene/search/BaseDocValuesSkipperTests.java

@@ -0,0 +1,196 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.search;
+
+import java.io.IOException;
+import org.apache.lucene.index.DocValuesSkipper;
+import org.apache.lucene.index.NumericDocValues;
+import org.apache.lucene.tests.util.LuceneTestCase;
+
+public abstract class BaseDocValuesSkipperTests extends LuceneTestCase {
+
+  /**
+   * Fake numeric doc values so that: - docs 0-256 all match - docs in 256-512 are all greater than
+   * queryMax - docs in 512-768 are all less than queryMin - docs in 768-1024 have some docs that
+   * match the range, others not - docs in 1024-2048 follow a similar pattern as docs in 0-1024
+   * except that not all docs have a - value
+   */
+  protected static NumericDocValues docValues(long queryMin, long queryMax) {
+    return new NumericDocValues() {
+
+      int doc = -1;
+
+      @Override
+      public boolean advanceExact(int target) throws IOException {
+        throw new UnsupportedOperationException();
+      }
+
+      @Override
+      public int docID() {
+        return doc;
+      }
+
+      @Override
+      public int nextDoc() throws IOException {
+        return advance(doc + 1);
+      }
+
+      @Override
+      public int advance(int target) throws IOException {
+        if (target < 1024) {
+          // dense up to 1024
+          return doc = target;
+        } else if (doc < 2047) {
+          // 50% docs have a value up to 2048
+          return doc = target + (target & 1);
+        } else {
+          return doc = DocIdSetIterator.NO_MORE_DOCS;
+        }
+      }
+
+      @Override
+      public long longValue() throws IOException {
+        int d = doc % 1024;
+        if (d < 128) {
+          return (queryMin + queryMax) >> 1;
+        } else if (d < 256) {
+          return queryMax + 1;
+        } else if (d < 512) {
+          return queryMin - 1;
+        } else {
+          return switch ((d / 2) % 3) {
+            case 0 -> queryMin - 1;
+            case 1 -> queryMax + 1;
+            case 2 -> (queryMin + queryMax) >> 1;
+            default -> throw new AssertionError();
+          };
+        }
+      }
+
+      @Override
+      public long cost() {
+        return 42;
+      }
+    };
+  }
+
+  /**
+   * Fake skipper over a NumericDocValues field built by an equivalent call to {@link
+   * #docValues(long, long)}
+   */
+  protected static DocValuesSkipper docValuesSkipper(
+      long queryMin, long queryMax, boolean doLevels) {
+    return new DocValuesSkipper() {
+
+      int doc = -1;
+
+      @Override
+      public void advance(int target) throws IOException {
+        doc = target;
+      }
+
+      @Override
+      public int numLevels() {
+        return doLevels ? 3 : 1;
+      }
+
+      @Override
+      public int minDocID(int level) {
+        int rangeLog = 9 - numLevels() + level;
+
+        // the level is the log2 of the interval
+        if (doc < 0) {
+          return -1;
+        } else if (doc >= 2048) {
+          return DocIdSetIterator.NO_MORE_DOCS;
+        } else {
+          int mask = (1 << rangeLog) - 1;
+          // prior multiple of 2^level
+          return doc & ~mask;
+        }
+      }
+
+      @Override
+      public int maxDocID(int level) {
+        int rangeLog = 9 - numLevels() + level;
+
+        int minDocID = minDocID(level);
+        return switch (minDocID) {
+          case -1 -> -1;
+          case DocIdSetIterator.NO_MORE_DOCS -> DocIdSetIterator.NO_MORE_DOCS;
+          default -> minDocID + (1 << rangeLog) - 1;
+        };
+      }
+
+      @Override
+      @SuppressWarnings("DuplicateBranches")
+      public long minValue(int level) {
+        int d = doc % 1024;
+        if (d < 128) {
+          return queryMin;
+        } else if (d < 256) {
+          return queryMax + 1;
+        } else if (d < 768) {
+          return queryMin - 1;
+        } else {
+          return queryMin - 1;
+        }
+      }
+
+      @Override
+      public long maxValue(int level) {
+        int d = doc % 1024;
+        if (d < 128) {
+          return queryMax;
+        } else if (d < 256) {
+          return queryMax + 1;
+        } else if (d < 768) {
+          return queryMin - 1;
+        } else {
+          return queryMax + 1;
+        }
+      }
+
+      @Override
+      public int docCount(int level) {
+        int rangeLog = 9 - numLevels() + level;
+
+        if (doc < 1024) {
+          return 1 << rangeLog;
+        } else {
+          // half docs have a value
+          return 1 << rangeLog >> 1;
+        }
+      }
+
+      @Override
+      public long minValue() {
+        return Long.MIN_VALUE;
+      }
+
+      @Override
+      public long maxValue() {
+        return Long.MAX_VALUE;
+      }
+
+      @Override
+      public int docCount() {
+        return 1024 + 1024 / 2;
+      }
+    };
+  }
+}