Solr
  1. Solr
  2. SOLR-5330

PerSegmentSingleValuedFaceting overwrites facet values

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.2.1
    • Fix Version/s: 4.5.1, 4.6, Trunk
    • Component/s: None
    • Labels:
      None

      Description

      I recently tried enabling facet.method=fcs for one of my indexes and found a significant performance improvement (with a large index, many facet values, and near-realtime updates). Unfortunately, the results were also wrong. Specifically, some facet values were being partially overwritten by other facet values. (That is, if I expected facet values like "abcdef" and "123", I would get a value like "123def".)

      Debugging through the code, it looks like the problem was in PerSegmentSingleValuedFaceting, specifically in the getFacetCounts method, when BytesRef val is shallow-copied from the temporary per-segment BytesRef. The byte array assigned to val is shared with the byte array for seg.tempBR, and is overwritten a few lines down by the call to seg.tenum.next().

      I managed to fix it locally by replacing the shallow copy with a deep copy.

      While I encountered this problem on Solr 4.2.1, I see that the code is identical in 4.5. Unless the behavior of TermsEnum.next() has changed, I believe this bug still exists.

      1. solr-5330.patch
        0.9 kB
        Michael Froh

        Activity

        Hide
        Erick Erickson added a comment -

        Michael:

        Thanks for the detailed explanation! Could I trouble you to go ahead and attach a patch? Don't worry about "polish", having your code change as a place to at least start (if not use entire) makes things easier for whoever picks this up...

        Show
        Erick Erickson added a comment - Michael: Thanks for the detailed explanation! Could I trouble you to go ahead and attach a patch? Don't worry about "polish", having your code change as a place to at least start (if not use entire) makes things easier for whoever picks this up...
        Hide
        Michael Froh added a comment -

        Patch attached

        Show
        Michael Froh added a comment - Patch attached
        Hide
        Yonik Seeley added a comment -

        Yikes, looks like this bug has been around a while. I'll take it.

        Show
        Yonik Seeley added a comment - Yikes, looks like this bug has been around a while. I'll take it.
        Hide
        Yonik Seeley added a comment -

        It's unfortunate that even the random faceting testing didn't catch this... I'm trying to fix that now.

        Show
        Yonik Seeley added a comment - It's unfortunate that even the random faceting testing didn't catch this... I'm trying to fix that now.
        Hide
        Yonik Seeley added a comment - - edited

        So I instrumented the faceting code like so:

                  seg.tempBR = seg.tenum.next();
        if (seg.tempBR.bytes == val.bytes) {
        System.err.println("##########SHARING DETECTED: val.offset="+val.offset + " val.length="+val.length + " new.offset="+seg.tempBR.offset + " new.length="+seg.tempBR.length);
        if (val.offset == seg.tempBR.offset) {
          System.err.println("!!!!!!SHARING USING SAME OFFSET");
        }
        

        And it detects tons of sharing (the returned bytesref still pointing to the same byte[]) of course... but the thing is, it never generates an invalid result. calling next() on the term enum never changes the bytes that were previously pointed to... it simply points to a different part of the same byte array. I can never detect a case where the original bytes are changed, thus invalidating the shallow copy.

        Example output:

        ##########SHARING DETECTED: val.offset=1 val.length=4 new.offset=6 new.length=4
        
        Show
        Yonik Seeley added a comment - - edited So I instrumented the faceting code like so: seg.tempBR = seg.tenum.next(); if (seg.tempBR.bytes == val.bytes) { System .err.println( "##########SHARING DETECTED: val.offset=" +val.offset + " val.length=" +val.length + " new .offset=" +seg.tempBR.offset + " new .length=" +seg.tempBR.length); if (val.offset == seg.tempBR.offset) { System .err.println( "!!!!!!SHARING USING SAME OFFSET" ); } And it detects tons of sharing (the returned bytesref still pointing to the same byte[]) of course... but the thing is, it never generates an invalid result. calling next() on the term enum never changes the bytes that were previously pointed to... it simply points to a different part of the same byte array. I can never detect a case where the original bytes are changed, thus invalidating the shallow copy. Example output: ##########SHARING DETECTED: val.offset=1 val.length=4 new .offset=6 new .length=4
        Hide
        ASF subversion and git services added a comment -

        Commit 1532900 from Yonik Seeley in branch 'dev/trunk'
        [ https://svn.apache.org/r1532900 ]

        SOLR-5330: make copy of term bytes before calling next

        Show
        ASF subversion and git services added a comment - Commit 1532900 from Yonik Seeley in branch 'dev/trunk' [ https://svn.apache.org/r1532900 ] SOLR-5330 : make copy of term bytes before calling next
        Hide
        ASF subversion and git services added a comment -

        Commit 1532903 from Yonik Seeley in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1532903 ]

        SOLR-5330: make copy of term bytes before calling next

        Show
        ASF subversion and git services added a comment - Commit 1532903 from Yonik Seeley in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1532903 ] SOLR-5330 : make copy of term bytes before calling next
        Hide
        ASF subversion and git services added a comment -

        Commit 1532905 from Yonik Seeley in branch 'dev/branches/lucene_solr_4_5'
        [ https://svn.apache.org/r1532905 ]

        SOLR-5330: make copy of term bytes before calling next

        Show
        ASF subversion and git services added a comment - Commit 1532905 from Yonik Seeley in branch 'dev/branches/lucene_solr_4_5' [ https://svn.apache.org/r1532905 ] SOLR-5330 : make copy of term bytes before calling next
        Hide
        Yonik Seeley added a comment -

        I never was able to reproduce... so I just fixed the issue anyway, making a copy of the bytes only instead of cloning the complete BytesRef.

        Show
        Yonik Seeley added a comment - I never was able to reproduce... so I just fixed the issue anyway, making a copy of the bytes only instead of cloning the complete BytesRef.

          People

          • Assignee:
            Yonik Seeley
            Reporter:
            Michael Froh
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development