diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index 3ce8e12813cc..f07a8fe4cbfd 100644 --- a/lucene/CHANGES.txt +++ b/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 diff --git a/lucene/analysis/common/src/java/org/apache/lucene/analysis/shingle/FixedShingleFilter.java b/lucene/analysis/common/src/java/org/apache/lucene/analysis/shingle/FixedShingleFilter.java index 4c82679f150b..3849d66615fd 100644 --- a/lucene/analysis/common/src/java/org/apache/lucene/analysis/shingle/FixedShingleFilter.java +++ b/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; + } } diff --git a/lucene/analysis/common/src/test/org/apache/lucene/analysis/shingle/TestFixedShingleFilter.java b/lucene/analysis/common/src/test/org/apache/lucene/analysis/shingle/TestFixedShingleFilter.java index 9c849e254cd7..191d6d73f197 100644 --- a/lucene/analysis/common/src/test/org/apache/lucene/analysis/shingle/TestFixedShingleFilter.java +++ b/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()); + } + } }