[GitHub] [lucene] jpountz commented on a change in pull request #224: LUCENE-10035: Simple text codec add multi level skip list data

[GitBox]

jpountz commented on a change in pull request #224:
URL: https://github.com/apache/lucene/pull/224#discussion_r694170097



##########
File path: 
lucene/codecs/src/java/org/apache/lucene/codecs/simpletext/SimpleTextFieldsReader.java
##########
@@ -357,19 +380,55 @@ public int nextDoc() throws IOException {
       }
     }
 
+    private int advanceTarget(int target) throws IOException {
+      if (seekTo > 0) {
+        in.seek(seekTo);
+        seekTo = -1;
+      }
+      assert docID() < target;
+      int doc;
+      do {
+        doc = readDoc();
+      } while (doc < target);
+      return doc;
+    }
+
     @Override
     public int advance(int target) throws IOException {
-      // Naive -- better to index skip data
-      return slowAdvance(target);
+      advanceShallow(target);
+      return advanceTarget(target);
     }
 
     @Override
     public long cost() {
       return cost;
     }
+
+    @Override
+    public void advanceShallow(int target) throws IOException {
+      if (skipReader.hasSkipList()) {

Review comment:
       Do we actually need to do this, since we seem to be already protecting 
against short postings lists that don't have skip data above?
   
   ```java
         if (docFreq <= SimpleTextSkipWriter.BLOCK_SIZE) {
           // no skip data
           return new SlowImpactsEnum(postings(null, flags));
         }
   ```

##########
File path: 
lucene/codecs/src/java/org/apache/lucene/codecs/simpletext/SimpleTextFieldsReader.java
##########
@@ -357,19 +380,55 @@ public int nextDoc() throws IOException {
       }
     }
 
+    private int advanceTarget(int target) throws IOException {
+      if (seekTo > 0) {
+        in.seek(seekTo);
+        seekTo = -1;
+      }
+      assert docID() < target;
+      int doc;
+      do {
+        doc = readDoc();
+      } while (doc < target);
+      return doc;
+    }
+
     @Override
     public int advance(int target) throws IOException {
-      // Naive -- better to index skip data
-      return slowAdvance(target);
+      advanceShallow(target);
+      return advanceTarget(target);
     }
 
     @Override
     public long cost() {
       return cost;
     }
+
+    @Override
+    public void advanceShallow(int target) throws IOException {
+      if (skipReader.hasSkipList()) {
+        if (target > nextSkipDoc) {
+          int numSkipped = skipReader.skipTo(target) + 1;
+          if (numSkipped > lastNumSkipped) {

Review comment:
       I don't understand this part, why do we need to track lastNumSkipped?




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