Lucene - Core
  1. Lucene - Core
  2. LUCENE-2645

False assertion of >0 position delta in StandardPostingsWriterImpl

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 4.0-ALPHA
    • Fix Version/s: 4.0-ALPHA
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      StandardPostingsWriterImpl line 159 is:

          assert delta > 0 || position == 0 || position == -1: "position=" + position + " lastPosition=" + lastPosition;            // not quite right (if pos=0 is repeated twice we don't catch it)
      

      I enable assertions when I run my unit tests and I've found this assertion to fail when delta is 0 which occurs when the same position value is sent in twice in arrow. Once I added RemoveDuplicatesTokenFilter, this problem went away. Should I really be forced to add this filter? I think delta >= 0 would be a better assertion.

        Activity

        Hide
        KuroSaka TeruHiko added a comment - - edited

        This is preventing one of unit tests for our Tokenizer/Filter product to fail. Although it looks as if the unit test case were failing, a closer look shows that the failure is from this this assertion inside the Lucene core.
        We have inspected the Token sequence that causes this assertion to fail, and it looks valid.
        The attached program can reproduce this false assertion failure. Please give this bug a higher priority, and remove this assertion.

        $ javac -cp lucene-core-4.0-SNAPSHOT.jar LuceneTrunkAssertErrorReproducer.java
        $ java -ea -cp .:lucene-core-4.0-SNAPSHOT.jar LuceneTrunkAssertErrorReproducer
        Exception in thread "main" java.lang.AssertionError: position=1 lastPosition=1
        	at org.apache.lucene.index.codecs.standard.StandardPostingsWriter.addPosition(StandardPostingsWriter.java:197)
        	at org.apache.lucene.index.FreqProxTermsWriterPerField.flush(FreqProxTermsWriterPerField.java:385)
        	at org.apache.lucene.index.FreqProxTermsWriter.flush(FreqProxTermsWriter.java:88)
        	at org.apache.lucene.index.TermsHash.flush(TermsHash.java:117)
        	at org.apache.lucene.index.DocInverter.flush(DocInverter.java:80)
        	at org.apache.lucene.index.DocFieldProcessor.flush(DocFieldProcessor.java:75)
        	at org.apache.lucene.index.DocumentsWriterPerThread.flush(DocumentsWriterPerThread.java:457)
        	at org.apache.lucene.index.DocumentsWriter.doFlush(DocumentsWriter.java:421)
        	at org.apache.lucene.index.DocumentsWriter.flushAllThreads(DocumentsWriter.java:548)
        	at org.apache.lucene.index.IndexWriter.doFlush(IndexWriter.java:2776)
        	at org.apache.lucene.index.IndexWriter.flush(IndexWriter.java:2753)
        	at org.apache.lucene.index.IndexWriter.prepareCommit(IndexWriter.java:2619)
        	at org.apache.lucene.index.IndexWriter.commitInternal(IndexWriter.java:2692)
        	at org.apache.lucene.index.IndexWriter.commit(IndexWriter.java:2674)
        	at org.apache.lucene.index.IndexWriter.commit(IndexWriter.java:2658)
        	at LuceneTrunkAssertErrorReproducer.main(LuceneTrunkAssertErrorReproducer.java:39)
        
        Show
        KuroSaka TeruHiko added a comment - - edited This is preventing one of unit tests for our Tokenizer/Filter product to fail. Although it looks as if the unit test case were failing, a closer look shows that the failure is from this this assertion inside the Lucene core. We have inspected the Token sequence that causes this assertion to fail, and it looks valid. The attached program can reproduce this false assertion failure. Please give this bug a higher priority, and remove this assertion. $ javac -cp lucene-core-4.0-SNAPSHOT.jar LuceneTrunkAssertErrorReproducer.java $ java -ea -cp .:lucene-core-4.0-SNAPSHOT.jar LuceneTrunkAssertErrorReproducer Exception in thread "main" java.lang.AssertionError: position=1 lastPosition=1 at org.apache.lucene.index.codecs.standard.StandardPostingsWriter.addPosition(StandardPostingsWriter.java:197) at org.apache.lucene.index.FreqProxTermsWriterPerField.flush(FreqProxTermsWriterPerField.java:385) at org.apache.lucene.index.FreqProxTermsWriter.flush(FreqProxTermsWriter.java:88) at org.apache.lucene.index.TermsHash.flush(TermsHash.java:117) at org.apache.lucene.index.DocInverter.flush(DocInverter.java:80) at org.apache.lucene.index.DocFieldProcessor.flush(DocFieldProcessor.java:75) at org.apache.lucene.index.DocumentsWriterPerThread.flush(DocumentsWriterPerThread.java:457) at org.apache.lucene.index.DocumentsWriter.doFlush(DocumentsWriter.java:421) at org.apache.lucene.index.DocumentsWriter.flushAllThreads(DocumentsWriter.java:548) at org.apache.lucene.index.IndexWriter.doFlush(IndexWriter.java:2776) at org.apache.lucene.index.IndexWriter.flush(IndexWriter.java:2753) at org.apache.lucene.index.IndexWriter.prepareCommit(IndexWriter.java:2619) at org.apache.lucene.index.IndexWriter.commitInternal(IndexWriter.java:2692) at org.apache.lucene.index.IndexWriter.commit(IndexWriter.java:2674) at org.apache.lucene.index.IndexWriter.commit(IndexWriter.java:2658) at LuceneTrunkAssertErrorReproducer.main(LuceneTrunkAssertErrorReproducer.java:39)
        Hide
        Michael McCandless added a comment -

        Bug only affects 4.0 (we don't have this assert in 3.x), but I'll backport the test case to 3.x to make sure.

        Show
        Michael McCandless added a comment - Bug only affects 4.0 (we don't have this assert in 3.x), but I'll backport the test case to 3.x to make sure.
        Hide
        Michael McCandless added a comment -

        Thanks Kurosaka!

        Show
        Michael McCandless added a comment - Thanks Kurosaka!
        Hide
        David Smiley added a comment -

        Thanks for the test Korusaka. I didn't realize my bug report last year that an assert condition's ">" should become ">=" was insufficient for a committer to simply make the 1-char change. I guess I should work on creating tests for nearly everything for my bug reports to get more traction. :-|

        Show
        David Smiley added a comment - Thanks for the test Korusaka. I didn't realize my bug report last year that an assert condition's ">" should become ">=" was insufficient for a committer to simply make the 1-char change. I guess I should work on creating tests for nearly everything for my bug reports to get more traction. :-|
        Hide
        Michael McCandless added a comment -

        While test cases are always welcome, they certainly are not necessary in a patch (Yonik's Law of Patches).

        Which issue had you opened before? Somehow it fell through the cracks... which, unfortunately, happens all the time in open-source. Best to bump/gently nag on important fixes...

        Show
        Michael McCandless added a comment - While test cases are always welcome, they certainly are not necessary in a patch (Yonik's Law of Patches). Which issue had you opened before? Somehow it fell through the cracks... which, unfortunately, happens all the time in open-source. Best to bump/gently nag on important fixes...
        Hide
        David Smiley added a comment -

        Which issue had you opened before?

        This one! – But if you want to give Korusaka credit for it because he submitted a patch then fine. He went the extra mile that I didn't think was necessary.

        Show
        David Smiley added a comment - Which issue had you opened before? This one! – But if you want to give Korusaka credit for it because he submitted a patch then fine. He went the extra mile that I didn't think was necessary.
        Hide
        Michael McCandless added a comment -

        D'oh! Woops I didn't see that you had opened this issue! And I missed it from last September... sorry

        I will add you to CHANGES.

        And no that extra mile is not necessary. Just some gentle nagging would help stuff not fall past the event horizons on our todo lists

        Show
        Michael McCandless added a comment - D'oh! Woops I didn't see that you had opened this issue! And I missed it from last September... sorry I will add you to CHANGES. And no that extra mile is not necessary. Just some gentle nagging would help stuff not fall past the event horizons on our todo lists
        Hide
        KuroSaka TeruHiko added a comment -

        Thank you, Michael, for quick fix, and David, for initially reporting this bug and giving me a credit

        Show
        KuroSaka TeruHiko added a comment - Thank you, Michael, for quick fix, and David, for initially reporting this bug and giving me a credit
        Hide
        Michael McCandless added a comment -

        Thank you both

        Show
        Michael McCandless added a comment - Thank you both

          People

          • Assignee:
            Michael McCandless
            Reporter:
            David Smiley
          • Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development