Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 6.5
    • Fix Version/s: 7.0, 6.6, 6.5.1
    • Component/s: core/index
    • Labels:
    • Lucene Fields:
      New, Patch Available
    • Flags:
      Patch, Important

      Description

      On released 6.5.0 version, flushing operation on sorted index throws ArrayIndexOutOfBoudException in NumericDocValuesWriter, NormValuesWriter and BinaryDocValuesWriter.

      New SortedXXXIterators are looking up documents in FixedBitSets or PackedValues based on remapped (sorted) document ID, without checking BitSets/Values ranges, which are based on original document IDs. Meanwhile FixedBitSets can be sparse not only in between documents with fields, but also after last (originally) document with given field (because writer's addValue() is not called for last documents without values for fields). So remapped (sorted) values range can have different useful values range and bounds checking should be done for remapped and not original ID.

      We were hit by this bug because our indexes are built from independent sources by partial updating fragments of documents, so there is always some documents without values in some fields.

      As I understand this bug, it shows when:

      • maxDoc is greater than 64 (64 is pre-allocated size for writers FixedBitSets)
      • some number of last taken documents have empty fields (so FixedBitSet won't be reallocated to maxDoc)

      Also, check for range of values for given field is now happening based on original ID (e.g. "upto < size"), so flushing can now lost some values, even without hitting AIOOBE.

      I will attach patch resolving issues with some writers; for other writers from LUCENE-7579, I am not sure if there are similar bugs in them; patch resolved our indexing issues, please check changes from LUCENE-7579 for confirmation of lack of additional bugs in other flush-sorting writers.

      1. LUCENE-7791.patch
        6 kB
        Jim Ferenczi
      2. sortflush.patch
        3 kB
        Przemysław Szeremiota
      3. sortflush-test.patch
        1 kB
        Przemysław Szeremiota

        Activity

        Hide
        mikemccand Michael McCandless added a comment -

        Whoa, nice catch! Is it possible to also add a test case in the patch showing the bug?

        Show
        mikemccand Michael McCandless added a comment - Whoa, nice catch! Is it possible to also add a test case in the patch showing the bug?
        Hide
        przemosz Przemysław Szeremiota added a comment -

        I'll try, but don't have anything ready, so I don't know if I manage it today

        Show
        przemosz Przemysław Szeremiota added a comment - I'll try, but don't have anything ready, so I don't know if I manage it today
        Hide
        przemosz Przemysław Szeremiota added a comment - - edited

        OK, test-patch: it fails on branch_6_5, and passes with patch; rudimentary test only for NumericDocValuesWriter, fails with AIOOBE:

           [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=TestIndexSorting -Dtests.method=testEmptyNonSortedIntField -Dtests.seed=B1B45F478095D85D -Dtests.slow=true -Dtests.locale=fr-BE -Dtests.timezone=Canada/Mountain -Dtests.asserts=true -Dtests.file.encoding=ISO-8859-1
           [junit4] FAILURE 0.02s | TestIndexSorting.testEmptyNonSortedIntField <<<
           [junit4]    > Throwable #1: java.lang.AssertionError: index=127, numBits=64
           [junit4]    > 	at __randomizedtesting.SeedInfo.seed([B1B45F478095D85D:EA0CCD4D0DFEC9E8]:0)
           [junit4]    > 	at org.apache.lucene.util.FixedBitSet.get(FixedBitSet.java:181)
           [junit4]    > 	at org.apache.lucene.index.NumericDocValuesWriter$SortingNumericIterator.next(NumericDocValuesWriter.java:257)
           [junit4]    > 	at org.apache.lucene.index.NumericDocValuesWriter$SortingNumericIterator.next(NumericDocValuesWriter.java:228)
           [junit4]    > 	at org.apache.lucene.codecs.memory.MemoryDocValuesConsumer.addNumericField(MemoryDocValuesConsumer.java:112)
           [junit4]    > 	at org.apache.lucene.codecs.memory.MemoryDocValuesConsumer.addNumericField(MemoryDocValuesConsumer.java:91)
           [junit4]    > 	at org.apache.lucene.codecs.perfield.PerFieldDocValuesFormat$FieldsWriter.addNumericField(PerFieldDocValuesFormat.java:111)
           [junit4]    > 	at org.apache.lucene.index.NumericDocValuesWriter.flush(NumericDocValuesWriter.java:96)
           [junit4]    > 	at org.apache.lucene.index.DefaultIndexingChain.writeDocValues(DefaultIndexingChain.java:258)
           [junit4]    > 	at org.apache.lucene.index.DefaultIndexingChain.flush(DefaultIndexingChain.java:142)
           [junit4]    > 	at org.apache.lucene.index.DocumentsWriterPerThread.flush(DocumentsWriterPerThread.java:444)
           [junit4]    > 	at org.apache.lucene.index.DocumentsWriter.doFlush(DocumentsWriter.java:539)
           [junit4]    > 	at org.apache.lucene.index.DocumentsWriter.flushAllThreads(DocumentsWriter.java:653)
           [junit4]    > 	at org.apache.lucene.index.IndexWriter.prepareCommitInternal(IndexWriter.java:3007)
           [junit4]    > 	at org.apache.lucene.index.IndexWriter.commitInternal(IndexWriter.java:3242)
           [junit4]    > 	at org.apache.lucene.index.IndexWriter.commit(IndexWriter.java:3205)
           [junit4]    > 	at org.apache.lucene.index.TestIndexSorting.testEmptyNonSortedIntField(TestIndexSorting.java:774)
           [junit4]    > 	at java.lang.Thread.run(Thread.java:745)
        
        Show
        przemosz Przemysław Szeremiota added a comment - - edited OK, test-patch: it fails on branch_6_5, and passes with patch; rudimentary test only for NumericDocValuesWriter, fails with AIOOBE: [junit4] 2> NOTE: reproduce with: ant test -Dtestcase=TestIndexSorting -Dtests.method=testEmptyNonSortedIntField -Dtests.seed=B1B45F478095D85D -Dtests.slow= true -Dtests.locale=fr-BE -Dtests.timezone=Canada/Mountain -Dtests.asserts= true -Dtests.file.encoding=ISO-8859-1 [junit4] FAILURE 0.02s | TestIndexSorting.testEmptyNonSortedIntField <<< [junit4] > Throwable #1: java.lang.AssertionError: index=127, numBits=64 [junit4] > at __randomizedtesting.SeedInfo.seed([B1B45F478095D85D:EA0CCD4D0DFEC9E8]:0) [junit4] > at org.apache.lucene.util.FixedBitSet.get(FixedBitSet.java:181) [junit4] > at org.apache.lucene.index.NumericDocValuesWriter$SortingNumericIterator.next(NumericDocValuesWriter.java:257) [junit4] > at org.apache.lucene.index.NumericDocValuesWriter$SortingNumericIterator.next(NumericDocValuesWriter.java:228) [junit4] > at org.apache.lucene.codecs.memory.MemoryDocValuesConsumer.addNumericField(MemoryDocValuesConsumer.java:112) [junit4] > at org.apache.lucene.codecs.memory.MemoryDocValuesConsumer.addNumericField(MemoryDocValuesConsumer.java:91) [junit4] > at org.apache.lucene.codecs.perfield.PerFieldDocValuesFormat$FieldsWriter.addNumericField(PerFieldDocValuesFormat.java:111) [junit4] > at org.apache.lucene.index.NumericDocValuesWriter.flush(NumericDocValuesWriter.java:96) [junit4] > at org.apache.lucene.index.DefaultIndexingChain.writeDocValues(DefaultIndexingChain.java:258) [junit4] > at org.apache.lucene.index.DefaultIndexingChain.flush(DefaultIndexingChain.java:142) [junit4] > at org.apache.lucene.index.DocumentsWriterPerThread.flush(DocumentsWriterPerThread.java:444) [junit4] > at org.apache.lucene.index.DocumentsWriter.doFlush(DocumentsWriter.java:539) [junit4] > at org.apache.lucene.index.DocumentsWriter.flushAllThreads(DocumentsWriter.java:653) [junit4] > at org.apache.lucene.index.IndexWriter.prepareCommitInternal(IndexWriter.java:3007) [junit4] > at org.apache.lucene.index.IndexWriter.commitInternal(IndexWriter.java:3242) [junit4] > at org.apache.lucene.index.IndexWriter.commit(IndexWriter.java:3205) [junit4] > at org.apache.lucene.index.TestIndexSorting.testEmptyNonSortedIntField(TestIndexSorting.java:774) [junit4] > at java.lang. Thread .run( Thread .java:745)
        Hide
        jimczi Jim Ferenczi added a comment -

        Great catch indeed !

        Also, check for range of values for given field is now happening based on original ID (e.g. "upto < size"), so flushing can now lost some values, even without hitting AIOOBE.

        I think it's ok since "size" corresponds to the max docID we have in the buffer so we cannot lost values here unless I am missing something ?
        So the only problem here is that we don't check if the remapped doc id is greater than the capacity of the bitset.

        please check changes from LUCENE-7579 for confirmation of lack of additional bugs in other flush-sorting writers.

        I did and the other doc value sorters do not use a bitset to handle missing values. I think we are safe with this patch.

        The patch looks good and the test too, this bug only appears in 6.x since the code is slightly different in 7.
        I'll merge shortly unless Michael McCandless has something to add here ?

        Show
        jimczi Jim Ferenczi added a comment - Great catch indeed ! Also, check for range of values for given field is now happening based on original ID (e.g. "upto < size"), so flushing can now lost some values, even without hitting AIOOBE. I think it's ok since "size" corresponds to the max docID we have in the buffer so we cannot lost values here unless I am missing something ? So the only problem here is that we don't check if the remapped doc id is greater than the capacity of the bitset. please check changes from LUCENE-7579 for confirmation of lack of additional bugs in other flush-sorting writers. I did and the other doc value sorters do not use a bitset to handle missing values. I think we are safe with this patch. The patch looks good and the test too, this bug only appears in 6.x since the code is slightly different in 7. I'll merge shortly unless Michael McCandless has something to add here ?
        Hide
        przemosz Przemysław Szeremiota added a comment -

        Maybe I worded it unclear:

        -      if (upto < size) {
        -        int oldID = sortMap.newToOld(upto);
        

        vs

        +      int oldID = sortMap.newToOld(upto);
        +      if (oldID < size) {
        

        size and oldID is bounded by actual number of values in buffer, but upto is not (not necessarily, because it is reordered); so i have had a hunch that it could lead to skip legitimate values if sorted (upto) ID is greater than value buffer size (but original wouldn't be); but I don't have a proof/test for it. Anyways, patch as it is should work

        Show
        przemosz Przemysław Szeremiota added a comment - Maybe I worded it unclear: - if (upto < size) { - int oldID = sortMap.newToOld(upto); vs + int oldID = sortMap.newToOld(upto); + if (oldID < size) { size and oldID is bounded by actual number of values in buffer, but upto is not (not necessarily, because it is reordered); so i have had a hunch that it could lead to skip legitimate values if sorted (upto) ID is greater than value buffer size (but original wouldn't be); but I don't have a proof/test for it. Anyways, patch as it is should work
        Hide
        jimczi Jim Ferenczi added a comment -

        Ok I see it now. The code assumes that size==maxDoc and that's of course not true for sparse field. Applying the sort on a sparse numeric doc values was also broken (throws AIOOBE) so I reused your patch and published another one which also fixes the sorting on sparse numeric doc values.
        Thanks so much for noticing this Przemysław Szeremiota !

        Show
        jimczi Jim Ferenczi added a comment - Ok I see it now. The code assumes that size==maxDoc and that's of course not true for sparse field. Applying the sort on a sparse numeric doc values was also broken (throws AIOOBE) so I reused your patch and published another one which also fixes the sorting on sparse numeric doc values. Thanks so much for noticing this Przemysław Szeremiota !
        Hide
        mikemccand Michael McCandless added a comment -

        +1 to merge.

        Maybe we should hold 6.5.1 RC2 for this?

        Show
        mikemccand Michael McCandless added a comment - +1 to merge. Maybe we should hold 6.5.1 RC2 for this?
        Hide
        jimczi Jim Ferenczi added a comment -

        Ok I'll merge to 6.x first, add the tests to master in order to ensure that this bug does not impact 7.x. I hope I can merge to 6.5 if the 6.5.1 RC2 can be rebuilt, thanks for raising this Michael McCandless.

        Show
        jimczi Jim Ferenczi added a comment - Ok I'll merge to 6.x first, add the tests to master in order to ensure that this bug does not impact 7.x. I hope I can merge to 6.5 if the 6.5.1 RC2 can be rebuilt, thanks for raising this Michael McCandless .
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 0902c9440ef10b02e909a6c58411356fea97bb5f in lucene-solr's branch refs/heads/branch_6x from Jim Ferenczi
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0902c94 ]

        LUCENE-7791: Fixed index sorting to work with sparse numeric and binary docvalues field.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 0902c9440ef10b02e909a6c58411356fea97bb5f in lucene-solr's branch refs/heads/branch_6x from Jim Ferenczi [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0902c94 ] LUCENE-7791 : Fixed index sorting to work with sparse numeric and binary docvalues field.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 7477dcd111b6950d4105623ad2cfe60faea463da in lucene-solr's branch refs/heads/branch_6_5 from Jim Ferenczi
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7477dcd ]

        LUCENE-7791: Fixed index sorting to work with sparse numeric and binary docvalues field.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 7477dcd111b6950d4105623ad2cfe60faea463da in lucene-solr's branch refs/heads/branch_6_5 from Jim Ferenczi [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7477dcd ] LUCENE-7791 : Fixed index sorting to work with sparse numeric and binary docvalues field.
        Hide
        jim.ferenczi Jim Ferenczi added a comment -

        Ok this is now merged to 6.x and 6.5 and the fix will be part of the 6.5.1 release.
        I still need to adapt the test for master.
        Thanks Przemysław Szeremiota and Michael McCandless !

        Show
        jim.ferenczi Jim Ferenczi added a comment - Ok this is now merged to 6.x and 6.5 and the fix will be part of the 6.5.1 release. I still need to adapt the test for master. Thanks Przemysław Szeremiota and Michael McCandless !
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 0404e37357b90b583d306074838d69c7074ce307 in lucene-solr's branch refs/heads/master from Jim Ferenczi
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0404e37 ]

        LUCENE-7791: add tests with index sorting and sparse docvalues fields

        Show
        jira-bot ASF subversion and git services added a comment - Commit 0404e37357b90b583d306074838d69c7074ce307 in lucene-solr's branch refs/heads/master from Jim Ferenczi [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0404e37 ] LUCENE-7791 : add tests with index sorting and sparse docvalues fields
        Hide
        przemosz Przemysław Szeremiota added a comment - - edited

        Jim Ferenczi What about SortingNumericIterator@NormValuesWriter? It throws too, and LUCENE-7791.patch misses it's fix?

        Show
        przemosz Przemysław Szeremiota added a comment - - edited Jim Ferenczi What about SortingNumericIterator@NormValuesWriter ? It throws too, and LUCENE-7791 .patch misses it's fix?
        Hide
        jim.ferenczi Jim Ferenczi added a comment -

        I don't know what happened but the fix for NormsValueWriter is not in my patch. I'll push the fix shortly with additional tests for this case, thanks for checking.

        Show
        jim.ferenczi Jim Ferenczi added a comment - I don't know what happened but the fix for NormsValueWriter is not in my patch. I'll push the fix shortly with additional tests for this case, thanks for checking.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit b7214a0a5dcaa4f487ae3e46d9f5c023c155e027 in lucene-solr's branch refs/heads/branch_6x from Jim Ferenczi
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b7214a0 ]

        LUCENE-7791: fix AIOOBE on NormValuesWriter too

        Show
        jira-bot ASF subversion and git services added a comment - Commit b7214a0a5dcaa4f487ae3e46d9f5c023c155e027 in lucene-solr's branch refs/heads/branch_6x from Jim Ferenczi [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b7214a0 ] LUCENE-7791 : fix AIOOBE on NormValuesWriter too
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit cd1f23c63abe03ae650c75ec8ccb37762806cc75 in lucene-solr's branch refs/heads/branch_6_5 from Jim Ferenczi
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=cd1f23c ]

        LUCENE-7791: fix AIOOBE on NormValuesWriter too

        Show
        jira-bot ASF subversion and git services added a comment - Commit cd1f23c63abe03ae650c75ec8ccb37762806cc75 in lucene-solr's branch refs/heads/branch_6_5 from Jim Ferenczi [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=cd1f23c ] LUCENE-7791 : fix AIOOBE on NormValuesWriter too
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 3316f47bcf110851ebf7f70b835027a9769bccd2 in lucene-solr's branch refs/heads/master from Jim Ferenczi
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3316f47 ]

        LUCENE-7791: add tests for index sorting with sparse text fields and norms

        Show
        jira-bot ASF subversion and git services added a comment - Commit 3316f47bcf110851ebf7f70b835027a9769bccd2 in lucene-solr's branch refs/heads/master from Jim Ferenczi [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3316f47 ] LUCENE-7791 : add tests for index sorting with sparse text fields and norms
        Hide
        przemosz Przemysław Szeremiota added a comment -

        Jim Ferenczi Michael McCandless Thank you both very much, great (and quick!!) job! I look forward to release

        Show
        przemosz Przemysław Szeremiota added a comment - Jim Ferenczi Michael McCandless Thank you both very much, great (and quick!!) job! I look forward to release
        Hide
        mikemccand Michael McCandless added a comment -
        Show
        mikemccand Michael McCandless added a comment - Thank you Przemysław Szeremiota !

          People

          • Assignee:
            Unassigned
            Reporter:
            przemosz Przemysław Szeremiota
          • Votes:
            3 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development