Extend the skip-index fast path to non-dense blocks and reuse the interval tracker

Author: slow-J

lucene/CHANGES.txt

@@ -291,7 +291,7 @@ Improvements
 
 Optimizations
 ---------------------
-(No changes)
+* GITHUB#16268: Use the doc-values skip index to skip per-doc value lookups in LongRangeFacetCutter. (Jakub Slowinski)
 
 Bug Fixes
 ---------------------
@@ -568,8 +568,6 @@ Optimizations
 
 * GITHUB#16228: Reuse scratch int[] for ordinal translation. (Tim Brooks)
 
-* GITHUB#16268: Use the doc-values skip index to skip per-doc value lookups for dense blocks in LongRangeFacetCutter. (Jakub Slowinski)
-
 Bug Fixes
 ---------------------
 * GITHUB#15754: Fix HTMLStripCharFilter to prevent tags from incorrectly consuming subsequent

lucene/sandbox/src/java/org/apache/lucene/sandbox/facet/cutters/ranges/IntervalTracker.java

@@ -36,6 +36,11 @@ interface IntervalTracker extends OrdinalIterator {
   /** clear recorded information on this tracker. * */
   void clear();
 
+  /**
+   * restart reading from the first recorded ordinal, to replay a {@link #freeze() frozen} tracker
+   */
+  void rewind();
+
   /** check if any data for the interval has been recorded * */
   boolean get(int index);
 
@@ -71,6 +76,12 @@ public void clear() {
       intervalsWithHit = 0;
     }
 
+    @Override
+    public void rewind() {
+      bitFrom = 0;
+      trackerState = 0;
+    }
+
     @Override
     public boolean get(int index) {
       return tracker.get(index);

lucene/sandbox/src/java/org/apache/lucene/sandbox/facet/cutters/ranges/LongRangeFacetCutter.java

@@ -27,7 +27,6 @@
 import org.apache.lucene.index.DocValuesSkipper;
 import org.apache.lucene.index.LeafReaderContext;
 import org.apache.lucene.index.NumericDocValues;
-import org.apache.lucene.index.SortedNumericDocValues;
 import org.apache.lucene.sandbox.facet.cutters.FacetCutter;
 import org.apache.lucene.sandbox.facet.cutters.LeafFacetCutter;
 import org.apache.lucene.search.LongValues;
@@ -48,7 +47,7 @@ public abstract class LongRangeFacetCutter implements FacetCutter {
   // TODO: refactor - weird that we have both multi and single here.
   final LongValuesSource singleValues;
 
-  // Field to read a DocValuesSkipper from on the single-valued path, or null when disabled.
+  // Field name whose skip index is used on the single-valued path, or null when faceting a source.
   final String skipField;
 
   final LongRangeAndPos[] sortedRanges;
@@ -153,25 +152,18 @@ public static LongRangeFacetCutter create(String field, LongRange[] longRanges)
   abstract List<InclusiveRange> buildElementaryIntervals();
 
   /**
-   * Returns the {@link DocValuesSkipper} for {@link #skipField} in this segment. Null when: no skip
-   * field is configured, the field has no skip index, or some doc in this segment has more than one
-   * value.
+   * Single-valued {@link LongValues} read directly from {@link #skipField} so its skip index can be
+   * used, or null when there is no skip field or the segment is multi-valued.
    */
-  final DocValuesSkipper maybeSkipper(LeafReaderContext context) throws IOException {
+  final LongValues singleValuedSkipField(LeafReaderContext context) throws IOException {
     if (skipField == null) {
       return null;
     }
-    SortedNumericDocValues sortedNumeric = DocValues.getSortedNumeric(context.reader(), skipField);
-    if (DocValues.unwrapSingleton(sortedNumeric) == null) {
-      return null;
-    }
-    return context.reader().getDocValuesSkipper(skipField);
-  }
-
-  /** Single-valued {@link LongValues} for {@link #skipField} in this segment. */
-  final LongValues skipFieldValues(LeafReaderContext context) throws IOException {
     NumericDocValues values =
         DocValues.unwrapSingleton(DocValues.getSortedNumeric(context.reader(), skipField));
+    if (values == null) {
+      return null;
+    }
     return new LongValues() {
       @Override
       public long longValue() throws IOException {
@@ -313,15 +305,20 @@ abstract static class LongRangeSingleValuedLeafFacetCutter implements LeafFacetC
 
     IntervalTracker requestedIntervalTracker;
 
-    // Skip index for the faceted field, or null when disabled.
     private final DocValuesSkipper skipper;
 
-    // Cached decision from advanceSkipper, valid for every doc up to (and including) upToInclusive:
-    // when upToSameInterval is true, all those docs map to elementary interval upToIntervalOrd.
+    // advanceSkipper's decisions for the current block; the fields below hold while doc <=
+    // upToInclusive, after which it runs again for the next block.
     private int upToInclusive = -1;
+    // Whether every value in the block maps to the single interval upToIntervalOrd.
     private boolean upToSameInterval;
+    // Whether every doc in the block has a value.
+    private boolean upToDense;
     private int upToIntervalOrd;
 
+    // Interval of the previous doc with a value, for replaying the tracker on a repeat.
+    private int previousIntervalOrd = -1;
+
     LongRangeSingleValuedLeafFacetCutter(LongValues longValues, long[] boundaries, int[] pos) {
       this(longValues, boundaries, pos, null);
     }
@@ -342,28 +339,34 @@ public boolean advanceExact(int doc) throws IOException {
 
       int intervalOrd;
       if (upToSameInterval) {
-        // We are inside a dense skip block that maps entirely to one elementary interval, so reuse
-        // the cached ordinal and skip the per-doc value lookup and binary search.
+        // Reuse the cached ordinal, skipping the binary search. A dense block also skips the value
+        // lookup, a sparse one still needs advanceExact to know whether this doc has a value.
+        if (upToDense == false && longValues.advanceExact(doc) == false) {
+          return false;
+        }
         intervalOrd = upToIntervalOrd;
       } else if (longValues.advanceExact(doc)) {
         intervalOrd = processValue(longValues.longValue());
       } else {
         return false;
       }
 
-      if (requestedIntervalTracker != null) {
-        requestedIntervalTracker.clear();
-      }
       elementaryIntervalOrd = intervalOrd;
-      maybeRollUp(requestedIntervalTracker);
       if (requestedIntervalTracker != null) {
-        requestedIntervalTracker.freeze();
+        if (skipper != null && intervalOrd == previousIntervalOrd) {
+          // Same interval as the previous doc, so replay its frozen rollup instead of rebuilding.
+          requestedIntervalTracker.rewind();
+        } else {
+          requestedIntervalTracker.clear();
+          maybeRollUp(requestedIntervalTracker);
+          requestedIntervalTracker.freeze();
+          previousIntervalOrd = intervalOrd;
+        }
       }
 
       return true;
     }
 
-    /** Mirrors {@code HistogramCollector#advanceSkipper}. */
     private void advanceSkipper(int doc) throws IOException {
       if (doc > skipper.maxDocID(0)) {
         skipper.advance(doc);
@@ -378,14 +381,9 @@ private void advanceSkipper(int doc) throws IOException {
       }
 
       upToInclusive = skipper.maxDocID(0);
-      // Now find the highest level where all docs have a value and map to the same interval.
+      // Climb to the highest level that still maps to a single interval.
       for (int level = 0; level < skipper.numLevels(); ++level) {
-        int totalDocsAtLevel = skipper.maxDocID(level) - skipper.minDocID(level) + 1;
-        if (skipper.docCount(level) != totalDocsAtLevel) {
-          // Some docs at this level have no value, so we can't resolve the whole block at once.
-          break;
-        }
-        // Long fields store raw values, the skipper's min/max map straight into the boundary space.
+        // Long fields store raw values, skipper's min/max maps straight into the boundary space.
         int minInterval = processValue(skipper.minValue(level));
         int maxInterval = processValue(skipper.maxValue(level));
         if (minInterval != maxInterval) {
@@ -394,6 +392,8 @@ private void advanceSkipper(int doc) throws IOException {
         upToInclusive = skipper.maxDocID(level);
         upToSameInterval = true;
         upToIntervalOrd = minInterval;
+        int totalDocsAtLevel = skipper.maxDocID(level) - skipper.minDocID(level) + 1;
+        upToDense = skipper.docCount(level) == totalDocsAtLevel;
       }
     }
 

lucene/sandbox/src/java/org/apache/lucene/sandbox/facet/cutters/ranges/NonOverlappingLongRangeFacetCutter.java

@@ -70,12 +70,11 @@ List<InclusiveRange> buildElementaryIntervals() {
 
   @Override
   public LeafFacetCutter createLeafCutter(LeafReaderContext context) throws IOException {
-    // Use the skip index when we can, otherwise fall back to the value source.
-    DocValuesSkipper skipper = maybeSkipper(context);
-    if (skipper != null) {
-      LongValues values = skipFieldValues(context);
+    LongValues skipFieldValues = singleValuedSkipField(context);
+    if (skipFieldValues != null) {
+      DocValuesSkipper skipper = context.reader().getDocValuesSkipper(skipField);
       return new NonOverlappingLongRangeSingleValueLeafFacetCutter(
-          values, boundaries, pos, skipper);
+          skipFieldValues, boundaries, pos, skipper);
     }
     if (singleValues != null) {
       LongValues values = singleValues.getValues(context, null);

lucene/sandbox/src/java/org/apache/lucene/sandbox/facet/cutters/ranges/OverlappingLongRangeFacetCutter.java

@@ -149,12 +149,11 @@ private static LongRangeNode split(int start, int end, List<InclusiveRange> elem
 
   @Override
   public LeafFacetCutter createLeafCutter(LeafReaderContext context) throws IOException {
-    // Use the skip index when we can, otherwise fall back to the value source.
-    DocValuesSkipper skipper = maybeSkipper(context);
-    if (skipper != null) {
-      LongValues values = skipFieldValues(context);
+    LongValues skipFieldValues = singleValuedSkipField(context);
+    if (skipFieldValues != null) {
+      DocValuesSkipper skipper = context.reader().getDocValuesSkipper(skipField);
       return new OverlappingSingleValuedRangeLeafFacetCutter(
-          values, boundaries, pos, requestedRangeCount, root, skipper);
+          skipFieldValues, boundaries, pos, requestedRangeCount, root, skipper);
     }
     if (singleValues != null) {
       LongValues values = singleValues.getValues(context, null);

lucene/sandbox/src/java/org/apache/lucene/sandbox/facet/utils/RangeFacetBuilderFactory.java

@@ -35,7 +35,6 @@ private RangeFacetBuilderFactory() {}
 
   /** Request long range facets for numeric field by name. */
   public static CommonFacetBuilder forLongRanges(String field, LongRange... ranges) {
-    // Pass the field by name so we can use its skip index when present.
     return new CommonFacetBuilder(
             field, LongRangeFacetCutter.create(field, ranges), new RangeOrdToLabel(ranges))
         .withSortByOrdinal();

lucene/sandbox/src/test/org/apache/lucene/sandbox/facet/TestRangeFacet.java

@@ -944,7 +944,6 @@ public void testSkipIndexEquivalenceExtremeValues() throws Exception {
             default -> TestUtil.nextLong(random(), -100, 100);
           };
       doc.add(NumericDocValuesField.indexedField("field", v));
-      doc.add(new LongPoint("point", v));
       w.addDocument(doc);
     }
 
@@ -998,8 +997,43 @@ public void testSkipIndexEquivalenceMultiValued() throws Exception {
     IOUtils.close(dir);
   }
 
-  // Asserts faceting "field" by name (skip index when available) matches faceting via a
-  // MultiLongValuesSource (no skip index), over random range sets including extreme bounds.
+  public void testSkipIndexEquivalenceFewValues() throws Exception {
+    Directory dir = newDirectory();
+    IndexWriterConfig iwc = newIndexWriterConfig();
+    iwc.setIndexSort(new Sort(new SortField("field", SortField.Type.LONG, false)));
+    iwc.setCodec(TestUtil.alwaysDocValuesFormat(new Lucene90DocValuesFormat(4)));
+    RandomIndexWriter w = new RandomIndexWriter(random(), dir, iwc);
+
+    int numDocs = atLeast(1000);
+    for (int i = 0; i < numDocs; i++) {
+      Document doc = new Document();
+      doc.add(NumericDocValuesField.indexedField("field", TestUtil.nextLong(random(), 0, 5)));
+      w.addDocument(doc);
+    }
+
+    assertSkipIndexEquivalence(w, "few-values");
+
+    w.close();
+    IOUtils.close(dir);
+  }
+
+  public void testSingleValuedNoSkipIndex() throws Exception {
+    Directory dir = newDirectory();
+    RandomIndexWriter w = new RandomIndexWriter(random(), dir);
+
+    int numDocs = atLeast(1000);
+    for (int i = 0; i < numDocs; i++) {
+      Document doc = new Document();
+      doc.add(new NumericDocValuesField("field", TestUtil.nextLong(random(), -100, 100)));
+      w.addDocument(doc);
+    }
+
+    assertSkipIndexEquivalence(w, "single-valued-no-skip");
+
+    w.close();
+    IOUtils.close(dir);
+  }
+
   private void assertSkipIndexEquivalence(RandomIndexWriter w, String desc) throws IOException {
     IndexReader r = w.getReader();
     try {