Lucene - Core
  1. Lucene - Core
  2. LUCENE-4423

DocumentStoredFieldVisitor.binaryField ignores offset and length

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major 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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        Adrien Grand added a comment -

        Committed on trunk branch 4.x and branch 4.0.

        Show
        Adrien Grand added a comment - Committed on trunk branch 4.x and branch 4.0.
        Hide
        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 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
        Uwe Schindler added a comment -

        Closed after release.

        Show
        Uwe Schindler added a comment - Closed after release.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development