Fix incorrect position increment generated by FixedShingleFilter

lucene/CHANGES.txt

@@ -530,6 +530,8 @@ Bug Fixes
 * GITHUB#16452: Fix over-allocation in MemoryAccountingBitsetCollectorManager.Result#bitSet, which
   is now sized to highestMatchedDoc + 1 instead of the full searched range. (Sasilekha R)
 
+* GITHUB#16577: Fix incorrect position increment generated by FixedShingleFilter. (Kishor Kharbas)
+
 Other
 ---------------------
 * GITHUB#16266: Remove deprecated search(Query, Collector) calls in QueryUtils by replacing

lucene/analysis/common/src/java/org/apache/lucene/analysis/shingle/FixedShingleFilter.java

@@ -57,6 +57,11 @@ public final class FixedShingleFilter extends GraphTokenFilter {
 
   private final CharTermAttribute buffer = new CharTermAttributeImpl();
 
+  // Accumulates position increments from base tokens until a shingle is emitted.  This is needed
+  // when a base token cannot produce a shingle but a later token, possibly stacked at the same
+  // position, can.
+  private int pendingPosInc;
+
   /**
    * Creates a FixedShingleFilter over an input token stream
    *
@@ -91,20 +96,25 @@ public FixedShingleFilter(
   @Override
   public boolean incrementToken() throws IOException {
 
-    int shinglePosInc, startOffset, endOffset;
+    int startOffset, endOffset;
+    // reset pendingPosInc to 0, since we are about to emit a shingle and any pending position
+    // increments
+    // have been accounted for in the position increment of previous shingle
+    pendingPosInc = 0;
 
     outer:
     while (true) {
       if (incrementGraph() == false) {
+        // no more routes from the base token, so move base token to next token in the input stream
         if (incrementBaseToken() == false) {
+          // no more base tokens, we are done
           return false;
         }
-        // starting a shingle at a new base position, use base position increment
-        shinglePosInc = incAtt.getPositionIncrement();
-      } else {
-        // starting a new shingle at the same base with a different graph, use a 0
-        // position increment
-        shinglePosInc = 0;
+        // Accumulate into pendingPosInc, the position increment of the new token read from input
+        // stream.
+        // pendingPosInc could be non-zero when we reach here without emitting a new token (from
+        // `continue outer` path).
+        pendingPosInc += incAtt.getPositionIncrement();
       }
 
       startOffset = offsetAtt.startOffset();
@@ -156,9 +166,21 @@ public boolean incrementToken() throws IOException {
     }
     clearAttributes();
     this.offsetAtt.setOffset(startOffset, endOffset);
-    this.incAtt.setPositionIncrement(shinglePosInc);
+    this.incAtt.setPositionIncrement(pendingPosInc);
     this.termAtt.setEmpty().append(buffer);
     this.typeAtt.setType("shingle");
     return true;
   }
+
+  @Override
+  public void end() throws IOException {
+    super.end();
+    this.incAtt.setPositionIncrement(this.incAtt.getPositionIncrement() + pendingPosInc);
+  }
+
+  @Override
+  public void reset() throws IOException {
+    super.reset();
+    pendingPosInc = 0;
+  }
 }

lucene/analysis/common/src/test/org/apache/lucene/analysis/shingle/TestFixedShingleFilter.java

@@ -20,6 +20,8 @@
 import java.io.IOException;
 import java.util.Iterator;
 import org.apache.lucene.analysis.TokenStream;
+import org.apache.lucene.analysis.tokenattributes.CharTermAttribute;
+import org.apache.lucene.analysis.tokenattributes.PositionIncrementAttribute;
 import org.apache.lucene.tests.analysis.BaseTokenStreamTestCase;
 import org.apache.lucene.tests.analysis.CannedTokenStream;
 import org.apache.lucene.tests.analysis.Token;
@@ -292,4 +294,42 @@ public void testWithGraphInput() throws IOException {
     assertTokenStreamContents(
         new FixedShingleFilter(it.next(), 2), new String[] {"fuz bar", "bar baz"});
   }
+
+  public void testPositionIncrementAfterSkippedGraphToken() throws IOException {
+    // 2-gram shingle cannot be formed starting a "Wi-Fi", so the next branch
+    // starting at "Wi" should have a position increment of 1, not 0.
+    TokenStream ts =
+        new CannedTokenStream(
+            new Token("Secure", 0, 6),
+            new Token("Wi-Fi", 1, 7, 12, 2),
+            new Token("Wi", 0, 7, 9),
+            new Token("Fi", 1, 10, 12));
+
+    assertTokenStreamContents(
+        new FixedShingleFilter(ts, 2),
+        new String[] {"Secure Wi-Fi", "Secure Wi", "Wi Fi"},
+        new int[] {1, 0, 1});
+  }
+
+  public void testFirstShinglePositionIncrementAfterSkippedGraphToken() throws IOException {
+    // Special case of previous test: "Wi-Fi" is the first token in the input stream.
+    // So first output token is emitted from branch starting at "Wi" which should have a position
+    // increment of 1, not 0.
+    TokenStream ts =
+        new CannedTokenStream(
+            new Token("Wi-Fi", 1, 0, 5, 2), new Token("Wi", 0, 0, 2), new Token("Fi", 1, 3, 5));
+
+    try (FixedShingleFilter filter = new FixedShingleFilter(ts, 2)) {
+      CharTermAttribute termAtt = filter.addAttribute(CharTermAttribute.class);
+      PositionIncrementAttribute posIncAtt = filter.addAttribute(PositionIncrementAttribute.class);
+
+      filter.reset();
+      assertTrue(filter.incrementToken());
+      assertEquals("Wi Fi", termAtt.toString());
+      assertEquals(1, posIncAtt.getPositionIncrement());
+      assertFalse(filter.incrementToken());
+      filter.end();
+      assertEquals(1, posIncAtt.getPositionIncrement());
+    }
+  }
 }