Lucene - Core
  1. Lucene - Core
  2. LUCENE-6055

PayloadAttribute.clone() should deep clone its BytesRef

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.10.3, 5.0, 6.0
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      PayloadAttribute.clone() does a shallow clone, unlike e.g. CharTermAttribute. Attributes should deep clone, otherwise capturing state isn't correct. In addition, both PA's and CTA's .clone() falsely documents that they do shallow cloning on purposes, so need to fix that too.

      1. LUCENE-6055.patch
        4 kB
        Shai Erera
      2. LUCENE-6055.patch
        3 kB
        Shai Erera

        Issue Links

          Activity

          Hide
          Shai Erera added a comment -

          Patch fixes PA.clone(). I added a test to TestAttributeSource as I didn't find a more suitable place. But if someone can suggest one, I will gladly move it.

          Other than that, I think it's ready.

          Show
          Shai Erera added a comment - Patch fixes PA.clone(). I added a test to TestAttributeSource as I didn't find a more suitable place. But if someone can suggest one, I will gladly move it. Other than that, I think it's ready.
          Hide
          Uwe Schindler added a comment -

          Looks good to me. I was not aware that the clone() javadocs was on the base class! Thanks for fixing the javadocs, too.

          Show
          Uwe Schindler added a comment - Looks good to me. I was not aware that the clone() javadocs was on the base class! Thanks for fixing the javadocs, too.
          Hide
          Uwe Schindler added a comment -

          We may need to add a short notice to MIGRATE.txt in branch_5x, because this is a backwards break that may impact some users and is not obvious to see.

          Show
          Uwe Schindler added a comment - We may need to add a short notice to MIGRATE.txt in branch_5x, because this is a backwards break that may impact some users and is not obvious to see.
          Hide
          Shai Erera added a comment -

          I noticed that PA.copyTo() wasn't implemented well - it called other.setPayload(payload.clone()), thereby sharing the underlying byte[] with both 'this' and 'other'. I fixed it and added a test case.

          Show
          Shai Erera added a comment - I noticed that PA.copyTo() wasn't implemented well - it called other.setPayload(payload.clone()), thereby sharing the underlying byte[] with both 'this' and 'other'. I fixed it and added a test case.
          Hide
          Shai Erera added a comment -

          We may need to add a short notice to MIGRATE.txt in branch_5x

          I'll add this:

          Index: lucene/MIGRATE.txt
          ===================================================================
          --- lucene/MIGRATE.txt  (revision 1637816)
          +++ lucene/MIGRATE.txt  (working copy)
          @@ -73,3 +73,9 @@
           situations where some documents do not have values for fields wrapped in other
           ValueSources.  Users who want to preserve the previous behavior may need to wrap
           their ValueSources in a "DefFunction" along with a ConstValueSource of "0.0".
          +
          +## PayloadAttributeImpl.clone() (LUCENE-6055)
          +
          +PayloadAttributeImpl.clone() did a shallow clone which was incorrect, and was
          +fixed to do a deep clone. If you require shallow cloning of the underlying bytes,
          +you should override PayloadAttributeImpl.clone() to do a shallow clone instead.
          
          Show
          Shai Erera added a comment - We may need to add a short notice to MIGRATE.txt in branch_5x I'll add this: Index: lucene/MIGRATE.txt =================================================================== --- lucene/MIGRATE.txt (revision 1637816) +++ lucene/MIGRATE.txt (working copy) @@ -73,3 +73,9 @@ situations where some documents do not have values for fields wrapped in other ValueSources. Users who want to preserve the previous behavior may need to wrap their ValueSources in a "DefFunction" along with a ConstValueSource of "0.0". + +## PayloadAttributeImpl.clone() (LUCENE-6055) + +PayloadAttributeImpl.clone() did a shallow clone which was incorrect, and was +fixed to do a deep clone. If you require shallow cloning of the underlying bytes, +you should override PayloadAttributeImpl.clone() to do a shallow clone instead.
          Hide
          Robert Muir added a comment -

          +1

          Show
          Robert Muir added a comment - +1
          Hide
          Ahmet Arslan added a comment -

          Is this something related to SOLR-5635 ?

          Show
          Ahmet Arslan added a comment - Is this something related to SOLR-5635 ?
          Hide
          Shai Erera added a comment -

          I don't know, but it could be. I.e. in regular indexing, the payload is consumed before the next document populates it ... unless you use TeeSinkTokenFilter and then the payload of the first may control the values for all. But it's hard to tell - need a concrete example.

          Show
          Shai Erera added a comment - I don't know, but it could be. I.e. in regular indexing, the payload is consumed before the next document populates it ... unless you use TeeSinkTokenFilter and then the payload of the first may control the values for all. But it's hard to tell - need a concrete example.
          Hide
          Uwe Schindler added a comment -

          +1 to commit. Thanks!

          Show
          Uwe Schindler added a comment - +1 to commit. Thanks!
          Hide
          ASF subversion and git services added a comment -

          Commit 1637945 from Shai Erera in branch 'dev/trunk'
          [ https://svn.apache.org/r1637945 ]

          LUCENE-6055: PayloadAttribute.clone() should deep clone its BytesRef

          Show
          ASF subversion and git services added a comment - Commit 1637945 from Shai Erera in branch 'dev/trunk' [ https://svn.apache.org/r1637945 ] LUCENE-6055 : PayloadAttribute.clone() should deep clone its BytesRef
          Hide
          ASF subversion and git services added a comment -

          Commit 1637946 from Shai Erera in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1637946 ]

          LUCENE-6055: PayloadAttribute.clone() should deep clone its BytesRef

          Show
          ASF subversion and git services added a comment - Commit 1637946 from Shai Erera in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1637946 ] LUCENE-6055 : PayloadAttribute.clone() should deep clone its BytesRef
          Hide
          Shai Erera added a comment -

          Committed to trunk and 5.x

          Show
          Shai Erera added a comment - Committed to trunk and 5.x
          Hide
          Robert Muir added a comment -

          reopen for backport

          Show
          Robert Muir added a comment - reopen for backport
          Hide
          ASF subversion and git services added a comment -

          Commit 1643417 from Michael McCandless in branch 'dev/branches/lucene_solr_4_10'
          [ https://svn.apache.org/r1643417 ]

          LUCENE-6055: PayloadAttribute.clone() should deep clone its BytesRef

          Show
          ASF subversion and git services added a comment - Commit 1643417 from Michael McCandless in branch 'dev/branches/lucene_solr_4_10' [ https://svn.apache.org/r1643417 ] LUCENE-6055 : PayloadAttribute.clone() should deep clone its BytesRef
          Hide
          Anshum Gupta added a comment -

          Bulk close after 5.0 release.

          Show
          Anshum Gupta added a comment - Bulk close after 5.0 release.

            People

            • Assignee:
              Shai Erera
              Reporter:
              Shai Erera
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development