Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-4423

DocumentStoredFieldVisitor.binaryField ignores offset and length

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0
    • Component/s: core/store
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      This is no problem with SimpleText and Lucene40 since in their cases, offset is always 0 and length the length of the byte[] array, but it might break with custom codecs.

      1. LUCENE-4423.patch
        0.6 kB
        Adrien Grand
      2. LUCENE-4423.patch
        1 kB
        Adrien Grand

        Activity

        Hide
        jpountz Adrien Grand added a comment -

        This patch should fix the issue, although I am not sure whether it should be the responsability of the StoredFieldVisitor or of the StoredFieldsReader to make sure that the byte[] is a copy (this patch assumes it it the responsability of StoredFieldVisitor).

        Moreover, do you think this issue should be a blocker for 4.0?

        Show
        jpountz Adrien Grand added a comment - This patch should fix the issue, although I am not sure whether it should be the responsability of the StoredFieldVisitor or of the StoredFieldsReader to make sure that the byte[] is a copy (this patch assumes it it the responsability of StoredFieldVisitor ). Moreover, do you think this issue should be a blocker for 4.0 ?
        Hide
        rcmuir Robert Muir added a comment -

        although I am not sure whether it should be the responsability of the StoredFieldVisitor or of the StoredFieldsReader to make sure that the byte[] is a copy (this patch assumes it it the responsability of StoredFieldVisitor).

        Can we separate this? This is a different issue (there are TODOs about that). I definitely dont want it mixed in with this bugfix.

        Show
        rcmuir Robert Muir added a comment - although I am not sure whether it should be the responsability of the StoredFieldVisitor or of the StoredFieldsReader to make sure that the byte[] is a copy (this patch assumes it it the responsability of StoredFieldVisitor). Can we separate this? This is a different issue (there are TODOs about that). I definitely dont want it mixed in with this bugfix.
        Hide
        jpountz Adrien Grand added a comment -

        Minimal patch that leaves the responsability to make the copy on the codec side.

        I created LUCENE-4425 to address the documentation issue.

        Show
        jpountz Adrien Grand added a comment - Minimal patch that leaves the responsability to make the copy on the codec side. I created LUCENE-4425 to address the documentation issue.
        Hide
        rcmuir Robert Muir added a comment -

        +1 for the patch.

        Unrelated, its also bogus we have a Field(byte[], int, int) ctor but no
        Field(byte[], int, int) setter. I guess i don't like the inconsistency:
        but its all sugar for BytesRef anyway.

        Show
        rcmuir Robert Muir added a comment - +1 for the patch. Unrelated, its also bogus we have a Field(byte[], int, int) ctor but no Field(byte[], int, int) setter. I guess i don't like the inconsistency: but its all sugar for BytesRef anyway.
        Hide
        rcmuir Robert Muir added a comment -

        Its also possible to add a test for this issue: we could do it either in AssertingCodec or AssertingAtomicReader,
        have it copy the bytes to a new byte[]

        { "BOGUS" + <original> }

        and set the offset/length.

        This way we know if any visitors are broken. (even though the current offset and length are superfluous imo,
        we should ensure visitors respect the contract).

        Show
        rcmuir Robert Muir added a comment - Its also possible to add a test for this issue: we could do it either in AssertingCodec or AssertingAtomicReader, have it copy the bytes to a new byte[] { "BOGUS" + <original> } and set the offset/length. This way we know if any visitors are broken. (even though the current offset and length are superfluous imo, we should ensure visitors respect the contract).
        Hide
        jpountz Adrien Grand added a comment -

        Committed on trunk branch 4.x and branch 4.0.

        Show
        jpountz Adrien Grand added a comment - Committed on trunk branch 4.x and branch 4.0.
        Hide
        commit-tag-bot Commit Tag Bot added a comment -

        [branch_4x commit] Adrien Grand
        http://svn.apache.org/viewvc?view=revision&revision=1389856

        LUCENE-4423: DocumentStoredFieldVisitor.binaryField ignores offset and length (merged from r1389850).

        Show
        commit-tag-bot Commit Tag Bot added a comment - [branch_4x commit] Adrien Grand http://svn.apache.org/viewvc?view=revision&revision=1389856 LUCENE-4423 : DocumentStoredFieldVisitor.binaryField ignores offset and length (merged from r1389850).
        Hide
        thetaphi Uwe Schindler added a comment -

        Closed after release.

        Show
        thetaphi Uwe Schindler added a comment - Closed after release.

          People

          • Assignee:
            jpountz Adrien Grand
            Reporter:
            jpountz Adrien Grand
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development